Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Status: Fixed » Active
Pol’s picture

Status: Active » Needs review
FileSize
3.85 KB

First attempt of a patch.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, should have caught all cases now.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -55,10 +54,16 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER) {
+    return parent::setDefinition($id, $definition); // TODO: Change the autogenerated stub

Sorry I'm not clear what the added TODO is for.

Pol’s picture

FileSize
3.81 KB

That's PHPStorm ! Fixed now.

catch’s picture

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -122,4 +125,18 @@ public function __sleep() {
+      throw new \InvalidArgumentException($type . " names must be lowercase: " . $id);

Sorry for piecemeal review. Shouldn't this use sprintf?

Pol’s picture

FileSize
3.82 KB

It's now using sprintf, indeed, it's better.

The last submitted patch, 2: issue_2503567.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: issue_2503567.patch, failed testing.

The last submitted patch, 5: issue_2503567.patch, failed testing.

cilefen’s picture

@Pol good work:

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -122,4 +125,18 @@ public function __sleep() {
    +   *   The Service ID or Parameter name.
    

    or definition name...

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -122,4 +125,18 @@ public function __sleep() {
    +  private function validateId($id, $type = 'Service ID') {
    

    I do not think $type should have a default value.

  3. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -122,4 +125,18 @@ public function __sleep() {
    +      throw new \InvalidArgumentException(sprintf("%s names must be lowercase: %s", $id, $type));
    

    I think the 2nd and 3rd sprintf() arguments are in the wrong order.

Noe_’s picture

FileSize
3.82 KB
668 bytes

Changed throw new \InvalidArgumentException(sprintf("%s names must be lowercase: %s", $id, $type)); to throw new \InvalidArgumentException(sprintf("%s names must be lowercase: %s", $type, $id));.

See patch and interdiff.

Noe_’s picture

Status: Needs work » Needs review

Forgot to set to "Needs review"

cilefen’s picture

@Noe_ Thanks, that should work better. Can you

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -66,9 +71,7 @@ public function register($id, $class = null) {
    +    $this->validateId($name, 'Parameter');
    

    Can you make this 'Service parameter'?

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -55,10 +54,16 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER) {
    +    $this->validateId($id);
    

    This should pass 'Service definition' as its second parameter.

  3. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -66,9 +71,7 @@ public function register($id, $class = null) {
    +    $this->validateId($name, 'Parameter');
    

    This should be 'Service parameter'.

  4. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -122,4 +125,18 @@ public function __sleep() {
    +  private function validateId($id, $type = 'Service ID') {
    

    Do not set a default for $type.

Tests will need to be modified accordingly and #11, comment 1 still needs doing.

Pol’s picture

FileSize
3.86 KB

Fix previous comments.

Status: Needs review » Needs work

The last submitted patch, 15: 2503567-15.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
4.78 KB

Forgot to change the expected error message. Fixed now.

Status: Needs review » Needs work

The last submitted patch, 17: 2503567-17.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
4.78 KB
Pol’s picture

FileSize
4.8 KB

Status: Needs review » Needs work

The last submitted patch, 20: 2503567-20.patch, failed testing.

The last submitted patch, 19: 2503567-19.patch, failed testing.

Status: Needs work » Needs review

Fabianx queued 20: 2503567-20.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 20: 2503567-20.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
4.8 KB
Fabianx’s picture

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -41,9 +42,7 @@ public function __construct(ParameterBagInterface $parameterBag = NULL) {
+    $this->validateId($id, 'Service definition');

@@ -55,10 +54,16 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER) {
+    $this->validateId($id, 'Service definition');
...
+    $this->validateId($id, 'Service parameter');

Should be Service ID here.

The $id parameter is not the definition.

cilefen’s picture

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -41,9 +42,7 @@ public function __construct(ParameterBagInterface $parameterBag = NULL) {
    +    $this->validateId($id, 'Service definition');
    

    This should be 'Service ID'.

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -55,10 +54,16 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER) {
    +    $this->validateId($id, 'Service parameter');
    

    This should be 'Service ID'.

  3. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -41,9 +42,7 @@ public function __construct(ParameterBagInterface $parameterBag = NULL) {
    +    $this->validateId($id, 'Service definition');
    

    This should be 'Service ID'. That's my mistake.

  4. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -122,4 +125,18 @@ public function __sleep() {
    +   * Validate Service ID or Parameter.
    

    'Validates service IDs and service parameters.'

  5. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -122,4 +125,18 @@ public function __sleep() {
    +  private function validateId($id, $type = '') {
    

    Do not set a default for $type.. I see no reason to. Is there a reason this function is private rather than protected?

The last submitted patch, 15: 2503567-15.patch, failed testing.

The last submitted patch, 17: 2503567-17.patch, failed testing.

The last submitted patch, 19: 2503567-19.patch, failed testing.

The last submitted patch, 20: 2503567-20.patch, failed testing.

mgifford’s picture

Status: Needs review » Needs work

Seems like this needs work.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Assigned: catch » Unassigned
Issue tags: +Bug Smash Initiative

This seems to be still applicable. Unassigning catch because it has been many a year since they were active here.

pooja saraah’s picture

Attached patch against #25 and #27

pooja saraah’s picture

Fixed Failed Commands against #45

andregp’s picture

Status: Needs work » Needs review
FileSize
3.77 KB
2.97 KB

Fixed the custom commands fails from #46 and tweaked the tests and code a bit to get it closer to #25 and #27.

Status: Needs review » Needs work

The last submitted patch, 47: 2503567-47.patch, failed testing. View results

longwave’s picture

As 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.

andregp’s picture

Some 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.

Wim Leers’s picture

This 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

  public function register($id, $class = NULL): Definition {
    if (strtolower($id) !== $id) {
      throw new \InvalidArgumentException("Service ID names must be lowercase: $id");
    }

… 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.