ParamConverterManager lazy services are broken and should use a service locator

Created on 26 March 2024, 6 months ago
Updated 28 June 2024, 2 months ago

Problem/Motivation

> $service = \Drupal::service('paramconverter_manager');
= Drupal\Core\ParamConverter\ParamConverterManager {#1601}

> (new ReflectionProperty($service, 'converters'))->getValue($service);
= [
    "drupal.proxy_original_service.paramconverter.views_ui" => Drupal\views_ui\ParamConverter\ViewUIConverter {#1608},
    "drupal.proxy_original_service.paramconverter.configentity_admin" => Drupal\Core\ParamConverter\AdminPathConfigEntityConverter {#1603},
    "paramconverter.entity" => Drupal\Core\ParamConverter\EntityConverter {#1606},
    "paramconverter.entity_revision" => Drupal\Core\ParamConverter\EntityRevisionParamConverter {#1607},
    "drupal.proxy_original_service.paramconverter.menu_link" => Drupal\Core\ParamConverter\MenuLinkPluginConverter {#1605},
    "drupal.proxy_original_service.node_preview" => Drupal\node\ParamConverter\NodePreviewConverter {#1609},
  ]

Services tagged paramconverter are often declared as lazy in order to try and avoid instantiating them early. But it appears the lazy flag is ignored and the actual service (not the proxy) is instantiated when ParamConverterManager is constructed - which happens on every page request.

paramconverter services should only be needed when required by a route so we should lazily load all of them instead of each service having to declare itself lazy (which appears to be broken anyway).

Steps to reproduce

See above, or also try adding a breakpoint to ViewUIConverter::__construct().

Proposed resolution

Convert ParamConverterManager to use a service locator.
Remove the lazy proxy services for paramconverter services.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Routing 

Last updated 8 days ago

Created by

🇬🇧United Kingdom longwave UK

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @longwave
  • 🇬🇧United Kingdom longwave UK

    By adding logging to Container::get():

        $this->loading[$id] = TRUE;
        file_put_contents('/tmp/container.log', implode(' ', array_keys($this->loading)). "\n", FILE_APPEND);
    
        try {
          $service = $this->createService($definition, $id);
        }
    

    we can see the chains of services that are created:

    router_listener router router.no_access_checks route_enhancer.param_conversion
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.views_ui
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.views_ui entity_type.manager
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.views_ui entity_type.manager string_translation
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.views_ui entity_type.manager string_translation string_translator.custom_strings
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.views_ui entity_type.manager entity.last_installed_schema.repository
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.views_ui tempstore.shared
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.views_ui tempstore.shared keyvalue.expirable
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.views_ui router.admin_context
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.views_ui entity.repository
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.views_ui entity.repository context.repository
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.configentity_admin
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager paramconverter.entity
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager paramconverter.entity_revision
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.menu_link
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.menu_link plugin.manager.menu.link
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.menu_link plugin.manager.menu.link Drupal\Core\Menu\MenuTreeStorageInterface
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.paramconverter.menu_link plugin.manager.menu.link menu_link.static.overrides
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.node_preview
    router_listener router router.no_access_checks route_enhancer.param_conversion paramconverter_manager drupal.proxy_original_service.node_preview tempstore.private
    

    If we lazily instantiate paramconverter_manager's services properly then we remove all those services from every single page load.

  • Status changed to Needs review 6 months ago
  • 🇬🇧United Kingdom longwave UK
  • Pipeline finished with Success
    6 months ago
    Total: 546s
    #130045
  • First commit to issue fork.
  • Pipeline finished with Failed
    4 months ago
    Total: 576s
    #159001
  • Status changed to Needs work 4 months ago
  • 🇺🇸United States smustgrave

    Rebased to make sure everything passed with all the changes from the last month.

    Unfortunately caused a large number of failures.

  • 🇮🇳India Pravesh_Poonia

    Resolve the issue with ParamConverterManager's lazy services by implementing a service locator approach. Currently, services tagged 'paramconverter' are declared as lazy but are instantiated early during ParamConverterManager construction, leading to performance issues. To address this, convert ParamConverterManager to utilize a service locator, ensuring paramconverter services are lazily loaded only when required by a route. Additionally, remove the lazy proxy services for paramconverter services to streamline the process and improve performance

  • 🇬🇧United Kingdom longwave UK

    It appears that #[AutowireLocator] does not support priority ordering. This means that the services are not sorted correctly when iterating through all of them and some fail. I traced through the compiler builder and in PriorityTaggedServiceTrait::findAndSortTaggedServices() the services are initially correctly sorted but when the final array is built the services are indexed by service name and the ordering is lost.

    https://github.com/symfony/symfony/issues/42506 implies this did work at some point - at least when using !tagged_locator instead of autowiring maybe - but it doesn't appear to here.

  • 🇬🇧United Kingdom longwave UK

    In fact, PriorityTaggedServiceTrait::findAndSortTaggedServices() does sort services correctly, but then later on ServiceLocatorTagPass::map() calls ksort($services) which destroys the order.

  • Status changed to Postponed 3 months ago
  • Status changed to Needs work 2 months ago
  • 🇬🇧United Kingdom longwave UK

    Upstream bug is fixed in Symfony 7.0.9

    https://github.com/symfony/symfony/pull/57581

Production build 0.71.5 2024