Problem/Motivation

Under certain circumstances when creating a new service with new service decorators you can end up with the error

The service "moduleb.service" has a dependency on a non-existent service "modulea.service".

Presently the only way to solve this problem is to create a Service Provider with the following code:

public function alter(ContainerBuilder $container) {
    if (!$container->hasDefinition('modulea.service')) {
      $container->removeDefinition('moduleb.service');
    }
  }

There are better ways to address this issue. One potential solution is https://www.drupal.org/project/drupal/issues/3111008 because that will allow service.yml files to set decoration_on_invalid to something other than exception (the default value).

This issue is to provide a smaller solution that will hopefully be easier to get passed.

Steps to reproduce

  1. Create a Drupal site. Create and install moduleb.
  2. Create modulea and create a service
  3. Add a service in moduleb that decorates the service from modulea

After those three steps nothing will function.

Proposed resolution

Add support for Symfony's decoration_on_invalid property in Drupal's YamlFileLoader.

CommentFileSizeAuthor
#13 drupal-3293926-12.patch2.4 KBkingdutch
#3 3293926-service-decorates-non-existant.patch844 bytesgeekygnr

Issue fork drupal-3293926

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

geekygnr created an issue. See original summary.

geekygnr’s picture

geekygnr commented

I am not really sure which version to apply this to so I picked the most recent but it exists in 9.x as well.

geekygnr’s picture

geekygnr commented
StatusFileSize
new 3293926-service-decorates-non-existant.patch844 bytes

Like I said this applies to multiple versions so I am going to test it against a 10.x branch and a 9.x branch.

geekygnr’s picture

geekygnr commented
Related issues: +#3111008: Use native Symfony YamlFileLoader
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Issue summary: View changes

Added a link to symfony docs for decoration_on_invalid.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented 
+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
@@ -301,7 +301,7 @@ private function parseDefinition($id, $service, $file)
+            $definition->setDecoratedService($service['decorates'], $renameId, $priority, ContainerInterface::IGNORE_ON_INVALID_REFERENCE);

Thinking out loud, could we instead (also?) support the `decoration_on_invalid` property in the services.yaml file and pass it along if provided, but otherwise keep the default?

Just thinking that the silent fail in some cases could be just as perplexing as getting the error and needing to know you need ServiceProvider as a way to get around it...

geekygnr’s picture

geekygnr commented

At present the YmlFileLoader class in Drupal is almost a direct copy and paste of its Symfony parent. I opted to suggest the smallest change here because there are other issues looking to remove the need to copy and paste a class and those seemed like better locations for working through supporting the properties and features not currently supported in Drupal.

Another course I considered was redoing the copy and paste of the YmlFileLoader but I haven't had the chance to fully understand what changes to the file are needed for Drupal.

driskell’s picture

driskell commented

The current patch here will not work in 9.5 due to:

Drupal\Core\DependencyInjection\Compiler\DependencySerializationTraitPass

It will still attempt to get the service for a decorated public service and cause an exception.

driskell’s picture

driskell commented

OK so now that https://www.drupal.org/project/drupal/issues/2531564 is merged, it's not a problem I think anymore

bircher’s picture

bircher
Location 🇨🇿
commented
Related issues: +#2863986: Allow updating modules with new service dependencies

isn't this similar to the problem we had in #2863986: Allow updating modules with new service dependencies?

I am all in favor of not crashing when services are decorated when drupal is installed or updated. But I think once the site is in a running state a missing service is a problem that should not be ignored. So I think the patch here is not a good option.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kingdutch made their first commit to this issue’s fork.

kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new drupal-3293926-12.patch2.4 KB

With the other improvements that were made in Drupal this can now be solved with Symfony's decoration_on_invalid tag directly. Since the related issue about replacing YamlFileLoader entirely has hit some speedbumps I think it's worth it to add decoration_on_invalid support to our own YamlFileLoader already.

We're seeing that the community is adopting "decorate over extend" more and more and this would be a big DX win by allowing a single property in a YAML file to replace an extra PHP class for optional decoration. It also lessens the gap to Symfony for any future replacement of the YamlFileLoader.

kingdutch opened merge request !9414

kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Issue summary: View changes

Forgot to update the IS.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs Review Queue Initiative

Would it be possible to get test coverage for the new exceptions

kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Status: Needs work » Needs review

Added tests :D

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Barcelona2024

The MR looks great to me. In general I think we should be bringing in improvements from YamlFileLoader to keep parity with Symfony features, and this is a nice and small self-contained step towards that.

I confirmed that the code added in this patch is a 1:1 copy from Symfony.

godotislate’s picture

godotislate
he/him
commented

Does there need to be test coverage that decoration_on_invalid does what it's expected to do for the values "exception" null and "ignore"? Or is it fine because it's directly copied from Symfony?

Also, do we need a CR documenting decoration_on_invalid can be set and how to use?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

setDecoratedService() is provided by Symfony, I don't think we need explicit coverage to check that Symfony works? We can rely on their tests to do that.

If we think a CR is useful then we can add one, but again given this is a Symfony feature then I'm not sure we need it.

godotislate’s picture

godotislate
he/him
commented

If we think a CR is useful then we can add one, but again given this is a Symfony feature then I'm not sure we need it.

Relying on the Symfony tests makes sense.

I only mention a CR because bringing a Symfony feature like autowiring was documented in a CR. Bringing in decoration_on_invalid probably isn't as significant as that, but since there is a delta between Drupal features and Symfony features, and there isn't any use yet of decoration_on_invalid in core, it might be useful for there to be documentation that the feature is available in Drupal.

kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs tests

I think a change record makes sense as per #21.

Moving to "Needs Review" for the CR to be reviewed and approved :) Implementation is unchanged and was approved in #18.

godotislate’s picture

godotislate
he/him
commented
Title: Service decorates non-existant service when module not installed » Error decorating non-existent service when inner service's module not installed
Status: Needs review » Reviewed & tested by the community

CR looks good. I also edited the issue title, mostly for a misspelling. Back to RTBC.

  • larowlan committed c0053e63 on 11.x 
    Issue #3293926 by kingdutch, geekygnr, godotislate, m4olivei, longwave:...

larowlan made their first commit to this issue’s fork.

larowlan opened merge request !9925

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 11.x

I think this qualifies as backport eligible to 10.4 under 'Critical API compatibility issues'

I've created an MR to run tests

larowlan closed merge request !9414

kingdutch’s picture

kingdutch
he/him
Primary language English
commented

Should this be backported to 10.4.0? I'm not entirely sure what upgrade paths are supported, can site builders go from 10.4.x to 11.0.x or are they expected to jump from 10.4.x to 11.1.x?

Contrib code that would use this would be compatible with ^10.4 || ^11.1 only, if we backport (i.e. would break on 11.0 since it's introduced as feature in 11.1).

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Yes I thought so, but wanted a green MR first

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Patch (to be ported) » Fixed

Committed to 10.4.x - thanks

  • larowlan committed a64662a3 on 10.4.x 
    Issue #3293926 by kingdutch, geekygnr, godotislate, m4olivei, longwave:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.