[TYPO3-core] RFC: #9574: Configuration enhancement for indexed_search

[Steffen Kamper]

Hi Michael,

first thanks for taking the time to review this patch.

Michael Stucki schrieb:
> Hi Steffen,
> 
> I reviewed the code, there are still many things that need improvement.
> 
> indexed_search/ext_typoscript_setup.txt:
> 
> 1.  plugin.tx_indexedsearch.templateFile
> 
> You cannot change the value for this! If you want to change it, you need
> to use a [compatVersion] condition and write a hook for the update
> wizard to mention this.
>

i thought about and every instance i know use the reccommended way: a 
copy of the template and assigned that one. So compat switch is for the 
rare people using indesxed search without any changes in settings.

I know it's important to mention that the template changed and an 
important part has to be copied to the custom template to get the output 
again, i ask myself if the update wizard is the best place to mention 
this (the update wizard can't copy this part to the custom template), 
but i see a need to inform about this.

> Additionally, we use to emphasize such changes with three !!! in the
> ChangeLog.
>

i fear that not all people read the ChangeLog of indexed search, or do 
you talk of the general ChangeLog?

> 2. #what to display
>    # if results should be outputted on different page use the pid here
> 
> => Please give more care to that. Always add a space after a comment
>    mark, and fix the English (page => pages).

nope, it's page (the result page), missing is a "a"

> 
> 3. "use the pid here"
> 
> What pid? Needs to be more clear.

Isn't that clear? Var name is resultPid, comment says that you have to 
enter the pid of the page only if you want to have the results on 
different page.

> 
> 4. plugin.tx_indexedsearch.show.advancedSearchLink
> 
> Value is changed. Must be done with a compatVersion, too.

i can revert this. These changes are not as relevant as you have this 
most important ones in the flexform.

> 
> 5. plugin.tx_indexedsearch.defaultResultsPerPage = 2
> 
> Can't be 2. The current default is 10 so unless there is a good reason
> to change it, please leave that as default.
> 
> Also, the name "defaultResultsPerPage" sounds odd to me. Why default? If
> you change this value, you also change the number of results per page,
> right? If so, remove the "default" from the name.
> 

2 is a typo, of coarse must be 10.
I only used the classvar "defaultResultNumber" for Typoscript, as the 
name sounded odd to me i changed resultNumber to resultsPerPage.
This value is used only one time in code, i fear that the result count 
calculation doesn't make usage of it, i will retest this.

> 6. ...pageBrowser.pageLinks_stdWrap.preCObject.dataWrap
> 
> I can't find a wrap in this value. Should this be "data" instead of
> "dataWrap"?
> 

yes, true. But using dataWrap will help users by changing this to insert 
a wrap here.

> 7. plugin.tx_indexedsearchbox
> 
> Should be named "tx_indexedsearch_box" (separate "box" from the rest).
>
ok


> 
> indexed_search/locallang.xml:
> 
> 1. flexform_indexed_search.sheetTitle
> 
> Consistency: "Indexed Search" is written with capital "S" although all
> other labels use lower-case.
> 
> But I think that using upper-case "S" would be correct anyways (it's
> part of a title). So if you like to fix it, you can probably do so. As
> far as I see, only BE labels would be affected, and their meaning
> wouldn't be changed anyway. So it should be ok if you leave your
> upper-case label and fix all others instead...
> 
>

i just adapted it from the title, was written with lower "s"
I will change all others to "Indexed Search" as you stated also. (Yes, 
it's BE only)

> indexed_search/pi/class.tx_indexedsearch.php:
> 
> 1.
> +205 foreach ($this->code as $c) {
> +206  switch (t3lib_div::strtolower($c)) {
> +207   case 'form':
> +208    if ($this->conf['code'] != 'form' && !$this->conf['showForm']) {
> +209     break;
> +210    }
> 
> I don't understand why the code must be "form" here. If it has a good
> reason, please add a comment to it.
> 

this has a simple reason. In flexform you can choose kind of output. So 
if you uncheck "Searchform" it's: $this->conf['showForm'] = 0
$this->conf['code'] != 'form' is for the first plugin (tx_indexedsearch) 
and not the indexed search box, which comes with code="form". So this 
check is needed, but not easy to comment ...


> +213 case 'rules':
> +214  if ($this->conf['showRules']) {
> 
> I don't like to set "code = rules" and then rules are only displayed if
> conf.showRules is set.
> 
> Suggestion:
> - Change the default code to "defaults"
> - Consider value of conf.showRules as long as the value is "defaults"
> - Ignore conf.showRules as soon as the "code" is different to that
> 
> (applies to all other cases of this statement, too)
>

I think you missed the flexform part. By default the codes are set, but 
you can uncheck it in flexform if you don't want to have the rules 
displayed. As i used 3 blocks for display (form, rules, results) i used 
them in TS(code) and in flexform. But i will think about it again if it 
can be done more easy.

The reason for splitting up was also to seperate these parts. Now it's 
possible to put the plugin 3 times on a page: one renders only the form, 
one only the rules and one only the results. So you are free to fit in 
custom designs.

> 3. // Browsing nav, bottom.
> 
> The line is repeated now. Most likely a typo?
> 
> 4. $pid = $this->conf['resultPid'] ? $this->conf['resultPid'] :
>                                                 $GLOBALS['TSFE']->id;
> 
> Remove $pid and set $this->conf['resultPid'] in ->initialize(), just
> like the rest.
> 
ok

> 
> General notes:
> 
> 1. I did not yet check the functionality of your patch. Will do so after
>    you have fixed all points mentioned above.
> 

I will make an updated patch these days.

> 2. I'm not quite happy that the template is changed, although I see the
>    need for it. In case someone has customized the template file and
>    points to that, the display will be broken after the update. I think
>    this should be mentioned somewhere, with a screenshot of the part
>    that must be added to that customized template.
>    Again I propose to use the update wizard for that.
>

See above - I see the need but don't know where the best place would be. 
Update Wizard normally does changes. Giving here an information without 
doing the changes sounds odd to me.
BTW - this gives me a new idea what other software often do: they 
implement a page "What's new". This could be done by the "about"-module 
which is the default startpage, can be filled with screenshots and with 
samples and can be accessed all the time.

> 3. Documentation is completely missing!!
> 
I will add the changes to documentation, i still wanted to wait if these 
changes will be accepted.

> Since there is too much stuff that needs to be fixed, I am -1 to the change.
> 
> However, I must say I like the idea of your change and hope to see this
> in 4.3.
> 
great!

vg Steffen