Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-30689: Registered Query Types for automatic configuration #2834

Merged
merged 5 commits into from
Oct 22, 2019

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Oct 18, 2019

Question Answer
JIRA issue EZP-30689
Required by #2651
Bug/Improvement yes
New feature no
Target version master (8.0@dev) for eZ Platform v3.0
BC breaks yes
Tests pass yes
Doc needed yes

This PR both provides an improvement and fixes broken behat test for Query Types by providing automatic configuration for them (autoconfigure: true causing auto-tagging of the services which implement \eZ\Publish\Core\QueryType\QueryType with ezpublish.query_type).

The reason for doing it now is unblocking Travis for #2651 to proceed with QA.

Breaking changes

Traversing through bundles to automatically register Query Types by the naming convention <Bundle>\QueryType\*QueryType has been dropped.

This convention imposed on a Developer specific directory structure, which is Symfony 2 mindset. Instead we provided automatic configuration for all services which implement QueryType interface.
It might seem like an additional setup step, but in fact, when we look at the services.yaml shipped with Symfony 4, we can see that the entire src directory has been mapped to App\* services with auto-configuration. It means that just placing that class in developer-chosen "autoconfigured" namespace is enough. It yields the same DX as it used to, without 50 lines of our code.

Doc

If the changes are accepted, the section "By convention" needs to be dropped or rephrased. The replacement convention now is auto-configuring service. The following section "Using a service tag" needs to be adjusted.
DIC config examples:

Recommended:

    App\Query\LatestContent:
        autoconfigure: true

By Service Tag:

    App\Query\LatestContent:
        tags:
            - {name: ezpublish.query_type}

By explicit alias instead of name (might be required in the future, if your QueryType needs to be lazy):

    App\Query\LatestContent:
        tags:
            - { name: ezpublish.query_type, alias: LatestContent }

TODO:

  • Register instances of \eZ\Publish\Core\QueryType\QueryType for auto-configuration.
  • Drop traversing through bundles to find Query Types (in favor of ^ auto-conf).
  • Implement DI tests.
  • Extract ezpublish.query_type" to a constant.
  • Fix Behat Query Types Feature scenario.
  • See if Behat test passes.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

doc/bc/changes-8.0.md Outdated Show resolved Hide resolved
@alongosz alongosz changed the title EZP-30689: Registered QueryType Controllers for auto-configuration EZP-30689: Registered Query Types for automatic configuration Oct 21, 2019
Provided automatic configuration for every service which implements
`eZ\Publish\Core\QueryType\QueryType`
by auto-tagging it with the "ezpublish.query_type" service tag.
doc/bc/changes-8.0.md Outdated Show resolved Hide resolved
@alongosz alongosz force-pushed the ezp-30689-query-type-autoconf branch from 3a15bcb to 1d86261 Compare October 22, 2019 09:25
@bdunogier
Copy link
Member

As long as the developer experience is the same in the end, I'm fine with the change.

@alongosz
Copy link
Member Author

As long as the developer experience is the same in the end, I'm fine with the change.

For e.g. Demo no change is required, which proves my point :)
Because in Sf4 all services are by default autoconfigured (the whole App\ namespace is mapped), I'd say experience is exactly the same. Unless someone has different setup, but we have no control over that (the same way we didn't have control over where developers have put their QueryTypes classes anyway).

@bdunogier
Copy link
Member

There is one thing that breaks: if you define query types in bundles other than App. Such as bundles an integrator would reuse over projects. Do we have ways to warn in 2.x that relying on the path alone is deprecated ?

@alongosz
Copy link
Member Author

There is one thing that breaks: if you define query types in bundles other than App. Such as bundles an integrator would reuse over projects.

Properly written bundle should register those as services anyway, but...

Do we have ways to warn in 2.x that relying on the path alone is deprecated ?

We could add that, reported it already as a bug (EZP-31064) because this PR is going to be merged in a moment to unblock QA on #2651.

@alongosz alongosz merged commit e166161 into ezsystems:master Oct 22, 2019
@alongosz alongosz deleted the ezp-30689-query-type-autoconf branch October 22, 2019 13:17
@alongosz
Copy link
Member Author

QA will be done on #2651.

@bdunogier
Copy link
Member

Properly written bundle should register those as services anyway, but...
Yes, "but.."
We did provide an alternative way to do so. Maybe it wasn't a smart move, but it was official. So maybe we can at least warn users upfront that they should modify their configuration and add autoconfigure to their querytypes ?

@alongosz
Copy link
Member Author

Yes, therefore added EZP-31064.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants