Package Manager blows up when it encounters already-unpacked recipes

Oh, man. I know exactly what's causing this. It's a core problem, though, so moving it there.

The backstory:

Package Manager is always trying to make sure you don't break your site in unexpected ways. Originally, we wrote it before recipes even existed. The assumption was that Composer should only ever modify code that is under its control, and that any attempt by Composer to overwrite code that it does not manage is a bug and would cause Bad Things to happen.

The bug:

The recipe unpacker plugin completely explodes that assumption. Its entire purpose is to take some code that is Composer-managed -- a recipe, in this case -- and detach it from Composer. This confuses the validator in question, and it throws an error. It is doing exactly what it should be doing; it's just that the thing it was written to do is now wrong.

It is really tricky to figure out how to proceed here. A couple of thoughts:

• We could remove the validator entirely, but do we really want to do that? The purpose of the validation is to prevent Composer from overwriting or otherwise interfering with custom modules that might have the same name, or path, as contrib ones. I suppose this is a legitimate thing for us to check, but I'm not sure if it is a realistic scenario or an edge case that we don't need to worry about too much.
• We could skip validation if the drupal/core-recipe-unpack Composer plugin is installed and enabled, and if the package that will be overwritten is a recipe, and if there's a configuration flag to ignore this situation. Probably the most robust fix, but complicated to implement and test.

I have a headache with pictures idea: add a configuration flag that allows this situation to be treated as a warning, not an error.

The flag will default to "off" by default, keeping the current (more conservative) behavior. If the flag is enabled, the message is simply logged and displayed as a regular warning.

Drupal CMS would enable this flag, on the assumption that most people are using the UI (Project Browser and Automatic Updates) to manage the code, rather than directly invoking Composer.

Since this would be adding a boolean config flag that defaults to FALSE, I imagine it probably wouldn't need an update path at all.

@catch had an even better suggestion: an allow-list, passed as a service parameter (since it's unlikely to need changing) of path patterns which can be overwritten. By default, it allows anything in recipes/ to be overwritten.

I like that a lot. It's focused, it needs no config or update path, and it solves the problem.

Fixed and tested. Just needs a little issue grooming and review.

Got permission from @phenaproxima to start a new pipeline and as suspected that test was a random failure.

Based on #4 I'm crediting @catch as well

Ran the test-only job https://git.drupalcode.org/issue/drupal-3582229/-/jobs/9171857 which showed the test coverage for the said change.

I'm not a package_manager expert so warning. But the changes made conceptually makes sense. Excellent comment that really helped me.

Nothing stands out to me, so will go on a limb here.

The proposed resolution in the IS mentions a "service parameter." Maybe we're getting terminology mixed up here, but my interpretation is of service parameter (which would be the same as "container parameter") is Symfony's: https://symfony.com/doc/current/service_container.html#service-parameters.

In which case, there'd be a parameter (with an array of regexs as the value) defined in package_manager.services.yml, then autowired into the service constructor using the Autowire attribute. Service parameters aren't the most intuitive thing to override, though. But we could do something similar to the core.moved_classes parameter, which gets merged with individual {module}.moved_classes parameters if they exist?

@catch can you clarify what you meant by "service parameters"?

I'm 95% sure @catch meant exactly what's implemented in the MR because, as I understand it, a "service parameter" is a parameter passed to the service. A "container parameter" would be the kind defined at the top of the services.yml file, and is globally available. It would not make much sense for this to be a container parameter, since this is so very specific to this one service in Package Manager.

Symonfy's documentation is very confusing on this point; as I read it, they are using "configuration parameters" (wat) to refer to container-level parameters.

In any event, we could commit this now to fix the bug (it is causing problems for Drupal CMS users right now), and if we decide to move the parameter to the container level, that could be done in a follow-up without any backwards compatibility concerns.

With that in mind, restoring RTBC but by all means kick it back if you still think we should await @catch's input. Since this is a bug, I'm hoping we can backport this all the way to 11.3.x.

The idea was to use a container parameter (same thing as a service parameter for me) since this seemed like a way to make this extensible via contrib if someone comes up with a recipe-a-like system that needs the same treatment. When I thought of that I also had thought it would be literal directory paths instead of regexes too.

Not really here this week.

@catch I converted it to a container parameter, but kept it as a regular expression.

The original version of this was using a more straightforward directory path and fnmatch(), but PHPStan (or was it PHPCS?) rejected that so I had to convert it to a regular expression.

I'm not sure we want it to be a list of path literals, because it needs to be somewhat freeform such that any recipe can be overwritten. I could switch to using a str_starts_with() check but that feels like a trap that would make it unnecessarily inflexible. Is this something you feel strongly about?

I believe all feedback for this one has been addressed.

Committed and pushed 607b832 to main and c800920 to 11.x. Thanks!

Since we're introducing a container parameter here, checking with other RMs about whether it can be backported to 11.3.x, so setting to be ported for now.

All good for 11.3.x: Committed 892b90d and pushed to 11.3.x. Thanks!