EZP-31588: As a Developer I want to configure search engine features with tags which follows same convention

[adamwojs]

Question	Answer
JIRA issue	EZP-31588
Type	improvement
Target eZ Platform version	v3.x
BC breaks	no
Tests pass	yes
Doc needed	yes

Tags used to register Search Engine features in the dependency injection container should be consistent and follow Symfony convention (snake case).

Commits between 73ae9f7 - d0ad33f contains refactoring of existing backward compatibility layer for ezplatform.field_type.* ezplatform.query_type.*.

Basically duplicated logic of iteration over service ids tagged wtith current and deprecated tag has been extracted to \eZ\Publish\Core\Base\Container\Compiler\TaggedServiceIdsIterator\BackwardCompatibleIterator.

Commit 76bce15 deprecates ezpublish.searchEngine and ezpublish.searchEngineIndexer in favour of ezplatform.search_engine and ezplatform.search_engine.indexer

Checklist:

• PR description is updated.
• Tests are implemented.
• Added code follows Coding Standards (use $ composer fix-cs).
• PR is ready for a review.

[alongosz]

I like the idea of common abstraction for BC layer.
What we can't have is a squash commit assigned to this specific Story in Release notes, but containing all this stuff (too unrelated). Could you, after review, but before QA, squash commits in such way we could merge it in a fast-forward way? One for BC layer, N for compiler passes related to specific features, and finally one for EZP-31588, prefixed with that ticket number.

[alongosz]

Does this captor demand a ransom? ;)

I'm not really sold on this naming, the most commonly used name for this kind of classes is Collector, I think. Unless this follows some kind of existing pattern?

General question: why do you need to add extra complexity to error handling? We rather rely on Monolog and Symfony Console handlers on that. Is it just to be able to process those errors in unit tests? For that we usually rely on warnings being converted to exceptions and validate them by ->expectException. Doesn't this work here?

[adamwojs]

I'm not really sold on this naming, the most commonly used name for this kind of classes is Collector, I think. Unless this follows some kind of existing pattern?

It was inspirated by https://www.javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/ArgumentCaptor.html but collector seems to be better naming choice,

General question: why do you need to add extra complexity to error handling? We rather rely on Monolog and Symfony Console handlers on that. Is it just to be able to process those errors in unit tests? For that we usually rely on warnings being converted to exceptions and validate them by ->expectException. Doesn't this work here?

There are dedicated ->expectDeprecation(...), ->expectDeprecationMessage(...) but for some reason they doesn't work. I need to figure out why.

[adamwojs]

->expectDeprecation(...) and ->expectDeprecationMessage(...) only works when the deprecation message is triggered without error suppression operator (`@`) e.g.

trigger_error('Lorem ipsum dolor...', E_USER_DEPRECATED)   // OK
@trigger_error('Lorem ipsum dolor...', E_USER_DEPRECATED)  // KO

[adamwojs]

What we can't have is a squash commit assigned to this specific Story in Release notes, but containing all this stuff (too unrelated). Could you, after review, but before QA, squash commits in such way we could merge it in a fast-forward way? One for BC layer, N for compiler passes related to specific features, and finally one for EZP-31588, prefixed with that ticket number.

Sure, but I would do only one commit for BackwardCompatibleterator + refactoring and one for EZP-31588. Plan for the future is to unify all available tags as separate PR's.

[alongosz]

What we can't have is a squash commit assigned to this specific Story in Release notes, but containing all this stuff (too unrelated). Could you, after review, but before QA, squash commits in such way we could merge it in a fast-forward way? One for BC layer, N for compiler passes related to specific features, and finally one for EZP-31588, prefixed with that ticket number.

Sure, but I would do only one commit for BackwardCompatibleterator + refactoring and one for EZP-31588. Plan for the future is to unify all available tags as separate PR's.

Fine by me. However please do not copy JIRA story format in commit message, it's not very useful for browsing commit history (again, it's a history, so it's the past anyway...[1] ;)). Release Notes contain full JIRA title, so it's fine.

[1] it's really not a mistake per se, it's a matter of keeping common convention. I hope at some point we'll achieve that...

OT: after "As a Developer" there should be no comma. I don't know who started to introduce this grammar mistake :-)

[mnocon]

I've verified that both verions of the tags (old and new) are correctly working for LSE, solr and elastic.

One thing that worries me - I can't get the deprecations warning to show up in logs.
Not sure if it's misconfiguration on my side, I've found this comment from @alongosz:

Unfortunately my tests show that @trigger_error doesn't get logged when building Symfony container.

From: ezsystems/ezpublish-kernel#2291 (~2 years ago)

I'm happy to discuss it tomorrow, I've used the prod monolog config: https://github.com/ezsystems/ezplatform/blob/master/config/packages/prod/monolog.yaml which contains a deprecations handler, but I might be missing something in config.

[alongosz]

One thing that worries me - I can't get the deprecations warning to show up in logs.
Not sure if it's misconfiguration on my side, I've found this comment from @alongosz:

Unfortunately my tests show that @trigger_error doesn't get logged when building Symfony container.

From: ezsystems/ezpublish-kernel#2291 (~2 years ago)

I'm happy to discuss it tomorrow, I've used the prod monolog config: https://github.com/ezsystems/ezplatform/blob/master/config/packages/prod/monolog.yaml which contains a deprecations handler, but I might be missing something in config.

@mnocon Monolog is not available during Container building because Logger is a Container service. Chicken and the egg problem.
Those should be available in Container deprecations log
./var/cache/<env>/<name><Env><Debug>ProjectContainerDeprecations.log
e.g: ./var/cache/dev/appDevDebugProjectContainerDeprecations.log

[mnocon]

@alongosz you're right! Found them in the mentioned file.

Thanks, this can be merged then (together with related PRs).