Contrib.social

Site specific overrides rely on implementation details

Open on Drupal.org →
Created on 24 June 2014, almost 11 years ago
Updated 18 February 2023, about 2 years ago

Problem/Motivation

DrupalKernel has this to say about the sites/X/services.yml

// Register site-specific service overrides.

However, it uses the same code as for every other service definition file. Essentially we rely on the implementation detail that a later service definition with the same name overrides an earlier one. This is really, really bad. If someone comes along and say changes the implementation to one using arrays and sorts then good luck figuring out which one wins.

Proposed resolution

Add an argument to YamlFileLoader::load(), parseDefinitions(), parseDefinition() enforcing an override and set it in DrupalKernel::buildContainer. The code does not need to change because at the end of parseDefinition() we have an explicit setDefinition (let's not dwell on why would parsing do a set instead of returning the definition and let the caller set it) which does the override already -- but that's the implementation detail that needs to be made explicit.

To clarify even further: this does not change anything in the contents of service.yml or the actual, current logic of YamlFileLoader. It, however, clarifies intent and makes it so that in a future version we do not introduce nasty and hard to find bugs. I attached a patch that just needs doxygen.

Note: normally we use PHP to alter definitions so this problem doesn't arise with modules because modules will not or at least should not override in YAML but use ->setDefinition calls.

🐛 Bug report
Status

Postponed: needs info

Version

9.5

Component
Bootstrap  →

Last updated 22 days ago

No maintainer
Created by

🇨🇦Canada chx

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

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment about 2 years ago →
    🇳🇿New Zealand quietone

    This was an issue of a bugsmash group triage meeting. I was the only one to look at this issue and I am not sure what the next step here is. So, I will ask if this is still something to implement. And is it just adding a test? It is not clear to me.

    Is this still relevant? What are the next steps?

  • Comment 11 months ago →
    🇺🇸United States Amber Himes Matz Portland, OR USA

    Looking at this issue again from Bug Smash. There's been no activity for over 1 year since it was marked as Postponed (maintainer needs more info), and could be potentially be closed.

    I am certainly no expert on this, but I did do some hunting for other issues related to YamlFileLoader and I learned that:

    Portions of this file are a direct copy of
// \Symfony\Component\DependencyInjection\Loader\YamlFileLoader"

    And, it seems, looking at other issues related to YamlFileLoader that the consensus is that Drupal's YamlFileLoader should really stay as close as possible to Symfony's. So I'm wondering if this issue should be closed a "won't fix"? Or possibly rolled in as a task with another feature request related to YamlFileLoader? Like ✨ Use native Symfony YamlLoader + Config Needs work maybe?

  • Status changed to Closed: outdated 2 months ago
  • Comment 2 months ago →
    🇺🇸United States smustgrave

    Since there hasn't been a follow up going on almost 2 years going to close out. If still an issue please reopen.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024