Make Package Manager more friendly to local development by allowing stages to operate directly on the codebase in some situations

So if I'm reading correctly this would be adding a direct-write mode when a flag is set, but only for project browser. The idea being you can composer require new dependencies and the simple act of doing so won't blow up your site, whereas update might.

Overall that sounds great I think.

Is there any situation where indirect dependencies might be updated as part of a composer require or does composer always just complain in that case?

Is there any situation where indirect dependencies might be updated as part of a composer require or does composer always just complain in that case?

Yes, Composer might update indirect dependencies as part of a require, depending on what the dependency solver spits out. So there is an element of risk to this, but I would say that:

• This is why the direct-write mode would be accessible only to developers.
• Project Browser could take steps to prevent indirect dependencies from being updated (Composer now accepts a --minimal-changes option, which tries to minimize changes to indirect dependencies; Package Manager could be changed to ensure that Project Browser can pass this option, or maybe Package Manager could just always pass the option regardless).
• If packages are properly respecting semantic versioning, and using sensible constraints for their own dependencies, then the risk is probably low enough to not be of concern to the non-technical user.

If packages are properly respecting semantic versioning, and using sensible constraints for their own dependencies (which is true for core, and probably widely true enough for contrib),

At least two fairly high usage contrib modules, maintained by different people, incremented hook_update_last_removed() in patch releases in the past month or so, preventing updates from e.g. 1.1.1 to 1.1.3.) Just the most recent example I can think of where a patch release led to a site that could not be updated.

--mininal sounds great to avoid this though.

Would it be possible to run --dry-run and detect if it includes updates? Would be annoyingly slow though, but maybe only when this mode is on?

Would it be possible to run --dry-run and detect if it includes updates? Would be annoyingly slow though, but maybe only when this mode is on?

Unfortunately, composer update doesn't give us any machine-readable output, with or without the --dry-run flag. The only way to detect what will happen, is to use the sandbox.

So as I said, there's some risk involved here, which is why direct-write wouldn't be enabled by default. I think it also makes sense for Package Manager to always pass the --minimal-changes flag to Composer to minimize the possibility of breaking changes being introduced (regardless of whether direct-write is enabled), but we should probably do that in a separate, blocking issue.

The reason Package Manager doesn't pass --minimal-changes is because I don't think it even existed when we were writing Package Manager. It was only introduced in Composer 2.7.0 (see https://getcomposer.org/changelog/2.7.0).

@catch wrote this:

So if I'm reading correctly this would be adding a direct-write mode when a flag is set, but only for project browser. The idea being you can composer require new dependencies and the simple act of doing so won't blow up your site, whereas update might.

Overall that sounds great I think.

I agree. There is already the package_manager_change_log entries that document what package manager requested and then separately logged what actually happened in composer, which helps providing a paper trail of what it did.

I also think we can trust composer itself probably better to not leave your composer.json/lock and vendor files in a half applied state than our own code that copies that over from the composer staging, so I don't see added risk there.

The MR looks good to me I think, --minimal-changes should reduce the amount of things that can go wrong.

Draft change record written: https://www.drupal.org/node/3506770 →

Appears to have a number of open threads, if those could be answered please.

Thanks all

@tedbow and I discussed this at DrupalCon. We agreed that we should just not dispatch PreApplyEvent and PostApplyEvent at all, since they don't make any sense if you have effectively already "applied" changes to the site by requiring some dependencies.

So we'll just document that, and test that those events are never dispatched in direct-write. But generally this is ready for review.

Some suggestions but think it looks pretty good!

I'm not sure how to test this without committing it, but how sure are you that this will successfully work with core updates where the code of package_manager itself can change?

@tedbow and I had a nice, long Zoom chat about #21 and the implications of it.

It is in the very nature of Package Manager to change the running code base. Right now, that only happens during the apply phase, and during that phase, Automatic Updates kicks the site into maintenance mode and tries to finish the current request as quickly as possible. This is precisely to mitigate the possibility of weird things happening while the running code base is changing itself.

The crux of the problem is that, conceptually, direct-write moves that risk from the apply phase, to the require phase. In direct-write mode, the apply phase effectively doesn't exist.

Ted and I felt that the right course of action, then, is for Package Manager to kick the site into maintenance mode when direct-write is happening. In other words -- when something is required in direct-write mode, go into maintenance mode, and emerge from it when the require has completed. The idea is to reduce the risk of weird things happening if the running code base is changed while, say, 1000 people are using the site. It can't completely eliminate the risk, but it would greatly reduce it.

Additionally, direct-write mode should result in a warning being flagged during status checks. That warning should state that direct-write mode can be risky because it changes the running code base. That is always true, and site owners should be aware of it. Direct-write mode is meant to make local development easier (i.e., a site with only one person using it), and it's inherently riskier in a production environment. Flagging a warning seems like a completely reasonable thing to do.

We think we could implement that change in a separate issue, after this one (which adds the direct-write API) is committed. That issue would be a stable blocker for Package Manager.

The second tricky thing that came up is the validators which are trying to compare the live site with the sandbox, by subscribing to PreApplyEvent. Most of these validators are just useless in direct-write mode, and their validation -- which may be valuable -- is just skipped. (For example, SupportedReleaseValidator checks to ensure you didn't just install some insecure or unsupported project into the sandbox.) How can we ensure that this validation is not lost? Even if the validation has already happened to your site, you as the site owner should still be aware of it as a problem that you need to address.

We're still not entirely sure what to do here, but Ted's idea is to shift a lot of that validation to StatusCheckEvent. Since there's no sandbox to compare the live site against, we'd have to record the state of the live site before anything else happened -- i.e., on or before PreCreateEvent is fired. Subscribers to StatusCheckEvent could then compare that recorded state to the current state of the site, to determine if anything untoward had happened during the require phase, regardless of whether it happened in a sandbox, or in the live site.

We could probably also do that in a stable-blocking follow-up issue.

Needs some additional test coverage for the maintenance mode and warning stuff I just added (based on what Ted and I talked about in #22), and I think this should definitely have manual testing before being committed.

OK - I've written tests for what was discussed in #22.

A few comments on the MR, I don't think there is anything too major in there.

Tried to rephrase some comments for added clarity.

Just went over this again with @phenaproxima, looks good!

Opened 📌 Move some Package Manager validation into the pre-require and status check event listeners Active to move some validation around as needed to support direct-write mode.

I feel pretty confident about this. Let's get the API in, then I'll test it against Drupal CMS and we can fix any glaring bugs. The addition of the API is harmless by itself, since it doesn't do anything unless extant sandbox managers actually opt into the capability. So the real test will come after contrib adopts this feature.

I read the MR, CR and updated credit. I re-worked the change record so that the improvement is the first thing mentioned. And I changed wording to be plain English.

Yes, that make sense. The wee change is an improvement.

Discussed 📌 Move some Package Manager validation into the pre-require and status check event listeners Active a bit with @phenaproxima in slack.

I had a quick look for validators that would be affected by this change (e.g. not run at all if this is used). The three I could find were:

EnabledExtensionsValidator
DuplicateInfoFileValidator.php
SandboxDatabaseUpdatesValidator

EnabledExtensionsValidator seems like the main problem.

https://www.drupal.org/project/pathauto/releases/8.x-1.11 → removes a ctools dependency, and will remove it as a composer dependency in a major release. This means that when sites eventually update to pathauto 2.x, they'll need to either composer require drupal/ctools or uninstall it (unless they happen to have another module installed that depends on it).

Automatic updates can't do any of that itself, so would need to rely on this validator to handle it (and then the site owner would be alerted they need to either install ctool explicitly or uninstall it before trying to update pathauto again, after which it would work), but that won't run in direct write mode. @phenaproxima pointed out that composer update --dry-run doesn't support machine readable output, so this might need a composer update --dry-run:json custom composer plugin in order to replace.

However, neither that, nor SandboxDatabaseUpdatesValidator are relevant to project browser. After writing that I realised it might be relevant, because if you install a project that depends on pathauto 2.x, and you have pathauto 1.x installed, then composer might update you to pathauto 2.x even though the command in composer require which could also result in ctools removal.

I think we need a follow-up to discuss this more, and maybe postpone implementing the API in project browser and especially automatic updates on that follow-up.

Bumped 📌 Move some Package Manager validation into the pre-require and status check event listeners Active to critical so it doesn't get forgotten, also added to package manager stable blockers. I think the scope is too big to do in a single issue here.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

catch → closed merge request !11076

Thanks @catch!

For whatever it's worth, I am in complete agreement that implementing this API needs to be done very carefully and thoughtfully. Project Browser should have a dedicated issue to add support for this, to fully think through the implications.

Automatic Updates implements at least three different sandbox managers, and each of them should get their own issue to discuss whether to support direct-write. The potential benefits are significant, but so are the risks.

The build test change here made it the slowest core test at over 3m20s, opened 📌 Split PackageInstallTest into two Active .