EZP-30664: Skipped indexing erroneous Content and logged all exceptions

[SerheyDolgushev]

Question	Answer
JIRA issue	EZP-30664
Bug/Improvement	yes
New feature	no
Target version	6.7
BC breaks	no
Tests pass	yes
Doc needed	no

Similar to ezsystems/ezplatform-solr-search-engine#139, but it is done for legacy search engine.

TODO:

• Implement feature / fix a bug.
• Implement tests.
• Fix new code according to Coding Standards ($ composer fix-cs).
• Ask for Code Review.

[SerheyDolgushev]

@alongosz @andrerom

[andrerom]

1. We should adapt IncrementalIndexer::updateSearchIndex() phpdoc to cover this as well.
2. Tentative: If @alongosz aggrees on EZP-30664: Skipped indexing erroneous Content and logged all exceptions ezplatform-solr-search-engine#139 (comment), this should be targeting 6.7.

[alongosz]

Indeed it might be worth to fix it on all LTS-es.

[andrerom]

Ok, seems all is clear then. So @SerheyDolgushev, change branch to 6.7 here and 1.5 on solr. And also add something like the following on IncrementalIndexer::updateSearchIndex() in this PR:

      * Then item is removed from index, if not it is added/updated.
+     *
+     * If generic unhandled exception is thrown, then item indexing is skipped and warning is logged.
      *
      * @param int[] $contentIds

[SerheyDolgushev]

@andrerom done.

[andrerom]

Unsure, I think we rather should put some focus on finishing #2221 as it does this more cleanly.

[alongosz]

Unsure, I think we rather should put some focus on finishing #2221 as it does this more cleanly.

Indeed that one provides much more improved solution... but probably won't solve the issue at hand. Please notice that for #2221 to work, custom 3rd party FT implementation must handle exceptions by throwing InvalidIndexDataException. So this implies much more work including convincing maintainers to upgrade their code ;)
But not saying no, just stating some context info.

[kmadejski]

custom 3rd party FT implementation must handle exceptions by throwing InvalidIndexDataException.

@alongosz not really, there is nothing against to catch generic \Exception in the indexer and pass the message into the collector as well. This way you don't have to convince anyone to anything. Anyway, let's move this discussion to the right place 🙂

[andrerom]

Ok, this adds value by not blocking indexing by default. And makes it much easier to read out errors during indexing to be able to solve those issues separately.

So sending this to QA.

[micszo]

@andrerom @alongosz Should this still go to 6.7 when we don't support 1.7 since January? :)

[lserwatka]

@micszo no, it should start from 6.13 if relevant.

[micszo]

@SerheyDolgushev could you please update base and rebase?

[andrerom]

@SerheyDolgushev remember to update base here first before you push (git rebase -i 6.13) rebased branch so travis don't get confused 👍

[SerheyDolgushev]

@andrerom I did:

1. Synced my local 6.7 and 6.13 from the upstream
2. Made git rebase --onto 6.13 6.7
3. Change the base in PR
4. Pushed

[micszo]

Would it make sense - is it possible - to add a newline to improve the style of the output?

[alongosz]

Would it make sense - is it possible - to add a newline to improve the style of the output?

@micszo This is a monolog log, not an output, so not really. Adding it here is a bit artificial and would result in blank lines in log files as well. AFAICS we don't have control over output in this case, it's a side effect of ProgressBar handling output next to Monolog. It might be possible to tweak ProgressBar, but that needs to be investigated. Please check on 7.5, I have a feeling it might look better there on Symfony 3.4.

[micszo]

Indeed, looks better on 2.5.

[micszo]

QA Approved on eZ Studio v1.13.5 & eZ Platform EE 2.5 with LSE.

[lserwatka]

@alongosz could you merge it up?

[alongosz]

Merged up to 7.5 as 4738799 and master as a75c0a9.

Thank you @SerheyDolgushev 🎉