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

Deprecate configuring tag names and service ids in compiler passes #40468

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Mar 15, 2021

Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? yes
Tickets -
License MIT
Doc PR -

This PR is aimed at reducing the code complexity by hardcoding the name of tags and service ids that compiler passes have to deal with.

I think making these names configurable only adds boilerplate and maintenance overhead for no benefit:

  • for the practice: the need to use a pass with a renamed tag/id should be extremely rare (do yo know any?)
  • for the theory: a decorating pass could still rename before/after the processing, so this use case is still supported.

Side note: I skipped updating changelog+upgrade files. This would be just noise to me (nobody uses this possibility anyway ;) )

src/Symfony/Component/Cache/composer.json Outdated Show resolved Hide resolved
chalasr added a commit that referenced this pull request Mar 15, 2021
This PR was merged into the 4.4 branch.

Discussion
----------

[Cache] fix test case name

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Should make #40468 green.

Commits
-------

e2dffea [Translation] fix test case name
@nicolas-grekas nicolas-grekas merged commit 7cc5cef into symfony:5.x Mar 16, 2021
@nicolas-grekas nicolas-grekas deleted the hardcode-tags branch March 16, 2021 09:12
nicolas-grekas added a commit that referenced this pull request Apr 1, 2021
…or (andrew-demb)

This PR was merged into the 5.3-dev branch.

Discussion
----------

Use symfony/deprecation-contracts instead of trigger_error

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | yes <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | - <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | - <!-- required for new features -->

As `symfony/deprecation-contracts` added to requirements with PR #40468, we can update `trigger_error` call in favor reusing deprecation abstractions

Commits
-------

1acc296 Use symfony/deprecation-contracts instead of trigger_error
@fabpot fabpot mentioned this pull request Apr 18, 2021
@derrabus
Copy link
Member

When resolving this deprecation, would you rather…

  • … inline all configurable strings?
  • … store them in private constants?
  • … keep the private properties to keep merges easy?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented May 21, 2021

… inline all configurable strings

derrabus added a commit that referenced this pull request Jul 2, 2021
…erListenersPass (derrabus)

This PR was merged into the 5.4 branch.

Discussion
----------

[EventDispatcher] Deprecate configuring tags on RegisterListenersPass

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | N/A
| License       | MIT
| Doc PR        | N/A

Follow-up to #40468.

Commits
-------

2c4effe Deprecate configuring tags on RegisterListenersPass
leofeyer pushed a commit to contao/contao that referenced this pull request Nov 16, 2021
Description
-----------

The `FragmentRendererPass` arguments have become deprecated in symfony/symfony#40468. Luckily, we don't actually need to use the compiler pass to get a list of services 😅

Commits
-------

e10c533 Replace FragmentRendererPass with tagged locator
ff626d6 CS
d270e6e CS
leofeyer pushed a commit to contao/core-bundle that referenced this pull request Nov 16, 2021
Description
-----------

The `FragmentRendererPass` arguments have become deprecated in symfony/symfony#40468. Luckily, we don't actually need to use the compiler pass to get a list of services 😅

Commits
-------

e10c5339 Replace FragmentRendererPass with tagged locator
ff626d63 CS
d270e6e7 CS
@veewee
Copy link
Contributor

veewee commented Mar 25, 2022

Looks like we are using it for custom event listener tags in grumphp configuration @nicolas-grekas 👯
Would have been nice to add these kind of things to the changelog nevertheless. Thanks!

@@ -26,6 +26,10 @@ class AddEventAliasesPass implements CompilerPassInterface

public function __construct(array $eventAliases, string $eventAliasesParameter = 'event_dispatcher.event_aliases')
{
if (1 < \func_num_args()) {
trigger_deprecation('symfony/event-dispatcher', '5.3', 'Configuring "%s" is deprecated.', __CLASS__);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like it still can be configured with $eventAliases, what do you think about mentioning $eventAliasesParameter in the deprecation message?

@donquixote
Copy link
Contributor

For the record:
We do use a custom tag name 'event_subscriber' in Drupal.
Currently we also use a custom compiler pass and a custom event dispatcher, but we are considering to move back to the symfony event dispatcher. https://www.drupal.org/project/drupal/issues/2909185
But we need to support the custom tag name for BC.
The reason for the custom tag name was made in the past, https://www.drupal.org/project/drupal/issues/1824400. It can be argued whether this was a good or bad decision.

While most of the events that will get dispatched by our 'dispatcher' service will be kernel events, we'll also be using the same tag for other non-kernel-related events.

I think the tag name 'kernel.event_dispatcher' is not meant to imply that all the events are "kernel events", so this justification seems questionable in hindsight.

for the practice: the need to use a pass with a renamed tag/id should be extremely rare (do yo know any?)

I think these customization options are typically not needed for projects that are built with symfony directly, but they can be useful for other frameworks or systems that use symfony classes and components selectively, instead of using symfony as a complete system.

A good reason for custom tag names and service ids could be to avoid conflicts with other tag names and service ids from non-symfony components, or to have multiple instances of e.g. the event dispatcher - for whichever reason.

for the theory: a decorating pass could still rename before/after the processing, so this use case is still supported.

Or add another pass to rename the tag on each service that uses it - if we don't care if the old tag is restored afterwards.

Still this is less clean than the parameter - but ok.

To be clear: I am not necessarily requesting to revert this, I just want to give feedback on the "nobody uses this".

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

Successfully merging this pull request may close these issues.

9 participants