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.
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.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | interdiff.2291589.2.5.txt | 653 bytes | yesct |
| #6 | 2291589_5.patch | 2.53 KB | yesct |
| #2 | 2291589_2.patch | 2.37 KB | chx |
Comments
Comment #1
chx commentedComment #2
chx commentedComment #3
chx commentedComment #4
chx commentedComment #5
dawehnerI guess we want to expand the documentation, sorry. Tagging for novice, so something will take it over.
Comment #6
yesct commentedLet's see if I got the description correct.
(I resisted "fixing" the formatting of the other @param above this added one. That can be a separate issue, but is out of scope for this issue.)
Comment #7
chx commentedYes, that's all, thanks!
Comment #8
dawehnerSo, how can this be RTBC if passing FALSE in now does still the same as you pass in TRUE ... so you certainly can't trust the signature at all.
Comment #9
chx commentedLet's default to TRUE and throw an exception on FALSE then?
Comment #10
dawehnerAs said on IRC I think the additional value of an API which pretends to act like we expect it, is not high.
Instead I would propose an explicit integration test coverage to ensure that later ones can override.
Comment #11
chx commentedIn theory, yes. In practice, people will change tests. Even when it has dire security consequences, they will change tests. I have seen that happening. But perhaps, with good comments to be careful in changing tests.
Comment #12
dawehnerWell, you know, even with your patch, nothing blocks people from changing the implementation, so adding tests add a level of safetyness, IMHO.
Comment #25
quietone commentedThis 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 #27
amber himes matzLooking 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:
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 #3111008: Use native Symfony YamlFileLoader maybe?
Comment #28
smustgrave commentedSince there hasn't been a follow up going on almost 2 years going to close out. If still an issue please reopen.