Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:5.xfromnicolas-grekas:hardcode-tags
Mar 16, 2021

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedMar 15, 2021•
edited
Loading

QA
Branch?5.x
Bug fix?no
New feature?no
Deprecations?yes
Tickets-
LicenseMIT
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 ;) )

yceruto reacted with thumbs up emoji
chalasr added a commit that referenced this pull requestMar 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-grekasnicolas-grekas merged commit7cc5cef intosymfony:5.xMar 16, 2021
@nicolas-grekasnicolas-grekas deleted the hardcode-tags branchMarch 16, 2021 09:12
nicolas-grekas added a commit that referenced this pull requestApr 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 abstractionsCommits-------1acc296 Use symfony/deprecation-contracts instead of trigger_error
@fabpotfabpot mentioned this pull requestApr 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
MemberAuthor

nicolas-grekas commentedMay 21, 2021•
edited
Loading

… inline all configurable strings

derrabus reacted with thumbs up emojisstok reacted with laugh emoji

derrabus added a commit that referenced this pull requestJul 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/AFollow-up to#40468.Commits-------2c4effe Deprecate configuring tags on RegisterListenersPass
leofeyer pushed a commit to contao/contao that referenced this pull requestNov 16, 2021
Description-----------The `FragmentRendererPass` arguments have become deprecated insymfony/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 locatorff626d6 CSd270e6e CS
leofeyer pushed a commit to contao/core-bundle that referenced this pull requestNov 16, 2021
Description-----------The `FragmentRendererPass` arguments have become deprecated insymfony/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 locatorff626d63 CSd270e6e7 CS
@veewee
Copy link
Contributor

veewee commentedMar 25, 2022•
edited
Loading

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

publicfunction__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".

andrew-demb and nicolas-grekas reacted with thumbs up emoji

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

Reviewers

@stofstofstof requested changes

@derrabusderrabusderrabus approved these changes

@ycerutoycerutoyceruto approved these changes

@chalasrchalasrchalasr approved these changes

@dunglasdunglasAwaiting requested review from dunglas

@srozesrozeAwaiting requested review from sroze

@xabbuhxabbuhAwaiting requested review from xabbuh

+1 more reviewer

@pamilpamilpamil left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

9 participants

@nicolas-grekas@derrabus@veewee@donquixote@stof@pamil@yceruto@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp