Use autoconfigure more in core

Created on 24 May 2024, about 1 month ago
Updated 12 June 2024, 12 days ago

Problem/Motivation

Autoconfigure (https://symfony.com/doc/current/service_container.html#services-autoconf...) allows to skip a lot of boilerplate with compiler passes etc. like automatically adding a service tag if a service implements a particular interface.

However, it only works if the individual services.yml specifies autoconfigure: true, so for existing compiler passes, can cause bc issues for modules that don't do this.

Can we somehow deprecate not having autoconfigure: true in services.yml? Or change Symfony's default to true while allowing people to explicitly disable it? Not sure, but opening this issue to discuss.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Fixed

Version

10.4 ✨

Component
BaseΒ  β†’

Last updated about 2 hours ago

Created by

πŸ‡¬πŸ‡§United Kingdom catch

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

Merge Requests

Comments & Activities

  • Issue created by @catch
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    This could be a novice issue if we are just adding autoconfigure: true to all the services.yml's

  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    I read the page you linked and solely based on how AsEventListener behaves -- which I have studied for the OOP hook patch -- I think you will be very disappointed at what autoconfigure: true actually provides with the components Drupal core currently uses: it is registered here aka in the full symfony/symfony package. twig.extension is added in Symfony\Bundle\TwigBundle\DependencyInjection\TwigExtension which does ship with Drupal core but as far as I am aware, Drupal doesn't use Symfony bundles at all.

    This is not to say it is completely useless -- but it does require some work, a bit of copy/paste at least to make it really useful.

  • πŸ‡¬πŸ‡§United Kingdom catch
  • Merge request !8287#3449569 Use autoconfigure more in core β†’ (Closed) created by kim.pepper
  • Status changed to Needs review 20 days ago
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    Given we are held up with concerns in ✨ Turn autowire & autoconfigure ON/OFF globally Needs review should we go ahead and do it the manual way here first?

    I was curious so I added autoconfigure: true and autowire: true to every services.yml file to see what happens.

  • Pipeline finished with Failed
    20 days ago
    Total: 561s
    #191196
  • Pipeline finished with Failed
    20 days ago
    Total: 506s
    #191233
  • πŸ‡¬πŸ‡§United Kingdom catch

    Yeah I think we should manually configure autoconfigure everywhere (what a sentence) in core whatever happens. The test failures in the MR show we might run into issues doing ✨ Turn autowire & autoconfigure ON/OFF globally Needs review for contrib too, but maybe it'll help us figure out what to do there.

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    I also added autowire: true everywhere too, which I think we should remove and just do autoconfigure: true first.

  • Pipeline finished with Canceled
    13 days ago
    Total: 686s
    #196736
  • Pipeline finished with Failed
    13 days ago
    Total: 543s
    #196743
  • Pipeline finished with Canceled
    13 days ago
    Total: 448s
    #196790
  • Pipeline finished with Success
    13 days ago
    Total: 511s
    #196793
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    I've toned this down to only add autoconfigure: true to non-test modules. I had to exclude the mysql.services.yml too for reasons I haven't looked into.

  • First commit to issue fork.
  • Status changed to RTBC 13 days ago
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    I agree with doing this as it sets a good precedent for contrib and custom code to copy, I also enabled it for announcements_feed which was missed from the MR.

    The reason that mysql fails is that mysql.views.cast_sql implements a Views interface, but not all tests enable Views, so the interface cannot be loaded during container rebuild in some tests. I don't think this is worth trying to work around - database drivers are probably a special case here.

  • Pipeline finished with Success
    13 days ago
    Total: 720s
    #197121
    • alexpott β†’ committed bbd5898b on 10.4.x
      Issue #3449569 by kim.pepper, longwave, catch: Use autoconfigure more in...
    • alexpott β†’ committed 0bfb4c79 on 11.0.x
      Issue #3449569 by kim.pepper, longwave, catch: Use autoconfigure more in...
    • alexpott β†’ committed 2588e097 on 11.x
      Issue #3449569 by kim.pepper, longwave, catch: Use autoconfigure more in...
  • Status changed to Fixed 13 days ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Committed and pushed 2588e097a9 to 11.x and 0bfb4c7978 to 11.0.x and bbd5898ba6 to 10.4.x. Thanks!

    Backported to 10.4.x as this is a task and not necessary to be a part of 10.3.x

Production build 0.69.0 2024