Service decorates non-existant service when module not installed

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.

Forgot to update the IS.

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

Added tests :D

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.

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?

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.

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.

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.

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

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

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

Yes I thought so, but wanted a green MR first

Committed to 10.4.x - thanks

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