Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Follow-up to #2498293: Only allow lowercase service and parameter names
Soft-Dependency for #2497243: Replace Symfony container with a Drupal one, stored in cache:
Problem/Motivation
We forgot to throw the exception in setDefinition(), which the YAML parser uses.
Proposed resolution
- Fix it.
Remaining tasks
- Fix it.
User interface changes
- None.
API changes
- None.
Comment | File | Size | Author |
---|---|---|---|
#50 | interdiff_2503567_47-49.txt | 2.27 KB | andregp |
#50 | 2503567-49.patch | 6.37 KB | andregp |
#47 | interdiff_2503567_46-47.txt | 2.97 KB | andregp |
#47 | 2503567-47.patch | 3.77 KB | andregp |
#46 | interdiff_45-46.txt | 1.48 KB | pooja saraah |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #2
PolFirst attempt of a patch.
Comment #3
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC, should have caught all cases now.
Comment #4
catchSorry I'm not clear what the added TODO is for.
Comment #5
PolThat's PHPStorm ! Fixed now.
Comment #6
catchSorry for piecemeal review. Shouldn't this use sprintf?
Comment #7
PolIt's now using sprintf, indeed, it's better.
Comment #11
cilefen CreditAttribution: cilefen commented@Pol good work:
or definition name...
I do not think $type should have a default value.
I think the 2nd and 3rd sprintf() arguments are in the wrong order.
Comment #12
Noe_ CreditAttribution: Noe_ at Devhouse Spindle commentedChanged
throw new \InvalidArgumentException(sprintf("%s names must be lowercase: %s", $id, $type));
tothrow new \InvalidArgumentException(sprintf("%s names must be lowercase: %s", $type, $id));
.See patch and interdiff.
Comment #13
Noe_ CreditAttribution: Noe_ at Devhouse Spindle commentedForgot to set to "Needs review"
Comment #14
cilefen CreditAttribution: cilefen commented@Noe_ Thanks, that should work better. Can you
Can you make this 'Service parameter'?
This should pass 'Service definition' as its second parameter.
This should be 'Service parameter'.
Do not set a default for $type.
Tests will need to be modified accordingly and #11, comment 1 still needs doing.
Comment #15
PolFix previous comments.
Comment #17
PolForgot to change the expected error message. Fixed now.
Comment #19
PolComment #20
PolComment #25
PolComment #26
Fabianx CreditAttribution: Fabianx as a volunteer commentedShould be Service ID here.
The $id parameter is not the definition.
Comment #27
cilefen CreditAttribution: cilefen commentedThis should be 'Service ID'.
This should be 'Service ID'.
This should be 'Service ID'. That's my mistake.
'Validates service IDs and service parameters.'
Do not set a default for $type.. I see no reason to. Is there a reason this function is private rather than protected?
Comment #32
mgiffordSeems like this needs work.
Comment #44
quietone CreditAttribution: quietone at PreviousNext commentedThis seems to be still applicable. Unassigning catch because it has been many a year since they were active here.
Comment #45
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commentedAttached patch against #25 and #27
Comment #46
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commentedFixed Failed Commands against #45
Comment #47
andregp CreditAttribution: andregp at CI&T commentedFixed the custom commands fails from #46 and tweaked the tests and code a bit to get it closer to #25 and #27.
Comment #49
longwaveAs the test fails show, we actually need uppercase letters in service names since we allowed autowiring. #3049525: Enable service autowiring by adding interface aliases to core service definitions will add many service aliases to core that use uppercase letters as class names will become valid service names.
Comment #50
andregp CreditAttribution: andregp at CI&T commentedSome yml files still have service names with uppercase characters which seems to be against #2498293: Only allow lowercase service and parameter names.I tried to fix them, but got stuck on the error:
1) Drupal\KernelTests\Core\DependencyInjection\AutowireTest::testAutowire
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "Drupal\KernelTests\Core\DependencyInjection\TestService".
/app/vendor/symfony/dependency-injection/ContainerBuilder.php:1030/app/vendor/symfony/dependency-injection/ContainerBuilder.php:600
/app/vendor/symfony/dependency-injection/ContainerBuilder.php:558
/app/core/tests/Drupal/KernelTests/Core/DependencyInjection/AutowireTest.php:27
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:726
Edit.: I'm sorry @longwave I posted and the page refreshed with your comment, if I had refreshed before posting I would have seen it.
Comment #51
Wim LeersThis is showing up explicitly in #3314137: Make Automatic Updates Drupal 10-compatible since #3284422: [META] Symfony 6.2 compatibility due to changes in Symfony 6.2's
FileLoader
— see #3314137-27: Make Automatic Updates Drupal 10-compatible and #3314137-28: Make Automatic Updates Drupal 10-compatible for details. In #3314137-29: Make Automatic Updates Drupal 10-compatible I discovered that even Drupal core's existing autowiring test technically violates… by somehow bypassing it:
😬
So I agree with @longwave in #49. Closing this. See y'all in #3049525: Enable service autowiring by adding interface aliases to core service definitions.