Problem/Motivation

Similar to #3436295: ParamConverterManager lazy services are broken and should use a service locator, PathProcessorManager injects a bunch of services when it is constructed. By adding logging to Container::get():

    file_put_contents('/tmp/container.log', str_repeat('>', count($this->loading)) . " $id\n", FILE_APPEND);

we can see the chains of services on an Umami install:

>> path_processor_manager
>>> path_processor_decode
>>> path_processor.image_styles
>>> path_processor_language
>>>> config.factory
>>>>> config.storage
>>>>>> cache.config
>>>>> config.typed
>>>>>> config.storage.schema
>>>>>> cache.discovery
>>>>>> validation.constraint
>>>>>>> container.namespaces
>>>>> language.config_factory_override
>>>>>> language.default
>>>> language_manager
>>>> language_negotiator
>>>>> plugin.manager.language_negotiation_method
>>>> current_user
>>>> language.config_subscriber
>>> path_processor_front
>>> path_processor.files
>>> path_alias.path_processor
>>>> path_alias.manager
>>>>> path_alias.repository
>>>>> path_alias.prefix_list

Steps to reproduce

Proposed resolution

Convert PathProcessorManager to use tagged iterators for the inbound and outbound processors. Sorting by priority will be handled by the iterator. Individual path processors will not be instantiated until path processing takes place.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3571046

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

longwave created an issue. See original summary.

longwave’s picture

Title: Use tagged iterators in PathProcessorManager » Use tagged iterators in PathProcessorManager and RouteProcessorManager
Status: Active » Needs review

RouteProcessorManager is very similar, let's just do the same there.

smustgrave’s picture

There a good way to test this one?

longwave’s picture

From a drush shell on 11.3.3:

$ ddev drush php
...
> \Drupal::getContainer()->initialized('path_processor.image_styles')
= true

After applying this MR and clearing cache:

$ ddev drush php
...
> \Drupal::getContainer()->initialized('path_processor.image_styles')
= false

This means that the individual path processor service is not instantiated - it will be done on demand when we actually need to process a path. This should be the same result for any service tagged as a path processor or route processor. This makes every single Drupal request a tiny bit more efficient.

smustgrave’s picture

Status: Needs review » Needs work
StatusFileSize
new214.02 KB

proof

Ran the command and got what was described. I did have to clear cache.

Moving to NW though as there's a merge conflict in the baseline.

longwave’s picture

Status: Needs work » Needs review

Rebased.

longwave’s picture

Also converted to autowiring and removed the constructor docblocks.

smustgrave’s picture

Status: Needs review » Needs work

Appears have unit test failures now.

longwave’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Believes JavaScript to be random but can’t rerun

sivaji_ganesh_jojodae made their first commit to this issue’s fork.

godotislate’s picture

Status: Reviewed & tested by the community » Needs review

One small question on the MR.

  • godotislate committed 5a0a3bb5 on main
    refactor: #3571046 Use tagged iterators in PathProcessorManager and...
godotislate’s picture

Status: Needs review » Patch (to be ported)

Since the applied suggestion was mine, and trivial and in test code, I merged this. Committed 5a0a3bb and pushed to main. Thanks!

This doesn't apply to 11.x because of phpstan baseline. I don't know if it needs to go to 11.x, but it'll need a backport if so.

smustgrave’s picture

Version: main » 11.x-dev
Status: Patch (to be ported) » Reviewed & tested by the community

Clean backport, actually applied pretty cleanly but all them had some fuzziness.

  • godotislate committed 21292df8 on 11.x
    refactor: #3571046 Use tagged iterators in PathProcessorManager and...
godotislate’s picture

Status: Reviewed & tested by the community » Fixed

Committed 21292df and pushed to 11.x Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.