Add Alpha level Experimental Package Manager module

Wim Leers → made their first commit to this issue’s fork.

The 3 failures on March 8:

• Drupal\Tests\package_manager\Kernel\PathExcluder\NodeModulesExcluderTest → legitimate failure → fixed by my commit just now, the test already passed! 😊
• Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testViewMode() → random failure

Next: porting all commits that have happened in the contrib module in the past ~60 hours! 🤓

Ted is working on an issue summary update! 🚀

📌 Add colinodell/psr-test-logger to core's dev dependencies Fixed landed 👍

📌 Add php-tuf/composer-stager to core dependencies and governance — for experimental Automatic Updates & Project Browser modules Needs work still needs to land 🤞

Adding the actual issue, summary no longer a place holder.

Note: As is mentioned in the summary, we will continue to automatically convert the contrib module to commits on this merge request.

So this issue is the place for reviews but will be making issues and commits on the contrib module to address that feedback. Those commits will automatically show back up here through the conversion

markup

General questions about this bit:

The codebase must be writable by the web server. Because this is intrinsic to the purpose of this module and the two modules that depend on it, this requirement is unlikely to change in the future. Although this will prevent Package Manager from working on some hosting environments, some users may use Package Manager in local or cloud environments where the file system is writable, even if their production environment is not. Project Browser is the most obvious example of this, because installing modules is common when building a site and is often done locally. In Automatic Updates, updating manually via the UI (rather than automatically during cron) may be done by some users in development environments, which is beneficial even if Automatic Updates cannot be used directly in the production environment.

With automatic updates, it's my understanding that unattended updates will be done on cron - this means potentially a different user to the webserver running cron with access to the filesystem, and webserver write access not being entirely required for that situation.

Can automatic updates be configured like that - unattended updates via non-web cron only?

If so, would it be feasible at some point to have queued installs and updates - i.e. you select what you want, it goes into a queue, cron runs the queue (this might need to be something like key/value expirable instead of a real queue so we could show pending status), package_manager installs your stuff, it shows up later. It would require a frequent server-side cron run but pretty sure cpanel and similar can do that. This might make things available to a slightly wider number of users although a fair bit of UX to overcome.

#13 @catch
Right now that is not possible but I have created a contrib issue for this functionality 📌 Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed . It should not be that difficult. I think we could do that via a Symfony Console command. This command would need to bootstrap Drupal as any module can provide event subscribers that validator operations as described in the summary. I have not kept up with the commands in core/lib/Drupal/Core/Command but it looks like bootstrapping in done here \Drupal\Core\Command\ServerCommand::boot so maybe this would not be something new for core.

I think #3351895 would not actually need any changes to Package Manager and only Automatic Updates but I guess we will see.

If so, would it be feasible at some point to have queued installs and updates - i.e. you select what you want, it goes into a queue, cron runs the queue

Yes that seems like it would be possible though of course Package Manager does not have this queue now but the queuing like this could also be done by Automatic Updates itself.

Additionally we could even do the staging of the install/update into the stage directory at the time the user does the selection. The cron operation could just do the apply() phase. This would have the advantage of being able to validate any problems that Package Manager or other modules check for and let the user know if there are problems that would stop the operation. For example the user might want to install Module X but because of complex Composer requirements that would actually force Module Y to update to a insecure version(Package Manager prevents this). Therefore user would see this right away and not have to wait till the cron job runs to try do this.

This also could be done in either Automatic Updates or Project Browser separately but eventually it might be better if some functionality lived in Package Manager. I would think this functionality would be more important in Automatic Updates than in Project Browser because this would allow more site to select security updates.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

#13:

This might make things available to a slightly wider number of users although a fair bit of UX to overcome.

Absolutely!

Is this a nice-to-have or a must-have?

Ideally we'd have (approximate) numbers for this. We probably can't get that. But maybe we have some experience with it? Do you think it's more like 1, 10 or 50%?

#14:

Additionally we could even do the staging of the install/update into the stage directory at the time the user does the selection.

I was first gonna disagree, but no, this is totally right, because the creation of the stage does not require the ability to write to the codebase, only to the filesystem (similar to just uploading files). That'd definitely make the UX concerns @catch raised less severe, because feedback would be happening together with user actions, instead of at a later time.

I would think this functionality would be more important in Automatic Updates than in Project Browser because this would allow more site to select security updates.

IMHO what @catch described would only provide an acceptable UX for Automatic Updates, not for Project Browser. Project Browser IMHO requires the ability to see the results of what you're doing at the time you're doing it, otherwise it'd be a nightmarish UX.

The requirement that the web server can write its own files is quite a bummer. We spent considerable effort ~15 years ago in Update Manager to let it work on sites that the web server can write to, and on ones that are more securely configured. For all the effort we're pouring into making a slick, new, "more secure" version of all this plumbing -- to have to tell everyone "but open yourself up to a bunch of wider escalation vulnerabilities by making sure your webserver can write to its own source code" seems like a major step backwards. 😢

I guess we don't really know the actual numbers of how many sites are more securely configured or not. Certainly the UX is more slick and easy if everything is writable. And if folks are really security conscious, they're probably not going to use any of this. So, to help with the long tail of sites not upgrading, most of them are probably on server-can-write configurations and it doesn't really matter. But it's unfortunate we can't play similar tricks in Package Manger that we are in Update Manager to have "sudo" step if the server can't directly write to its own files...

This is a bit of an unsettling sentence:

And if folks are really security conscious, they're probably not going to use any of this.

That makes it sound to me like Automatic Updates will be (more or less) just a toy? In my mind, using Automatic Updates should offer the highest level of security, and not require an insecure set up.

If it does ... well then it kind of defeats the whole purpose of installing it in the first place, as I see it. Or maybe you can't have one (Automatic Updates) without the other (insecure set up)?

Automatic Updates use case with protected file system

Re #16 to handle the case where the file system is not writeable by the web server we are working on 📌 Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed . It would be for Automatic Updates cron updates use case not direct operations through the UI as provided by Project Browser. Of course this would require setting up and outside cron job

I guess we don't really know the actual numbers of how many sites are more securely configured or not.

This is hard to tell and of course our numbers of which versions of Drupal people are running is dependent on those sites using the Update module but I looked at reported usage recently and found

Here are numbers for sites running secure and insecure versions of Drupal:
Secure 9.5 70,489
Insecure 9.5 84,904
Secure 10.0 9511
Insecure 10.0 9,797

So in both case more sites are running insecure than secure versions of Drupal core.
Of those sites running insecure versions some probably aren't configured to have file system protected anyways so having cron Automatic Updates would probably be a security improvement.

Of the sites where they are running insecure versions and the file system is protected it maybe a choice of whether those admins want to make the files system writable as way to keep core up to date. I talked with a few agencies where they just cannot afford to keep the sites for their many clients up to date and would be willing to configure those hosting to be compatible with Package Manager because they think keeping the sites on secure versions are more important. But my discussions were before we considered #3351895 so in that case the file system could still be protected but a outside cron job would have to be set up.

With @catch's idea in #13 It might also be possible to provide a UI to select Updates that would then be installed via cron. With specific command as suggested in #3351895. The lag between selecting the updates and having them installed could be very minimal as the server cron job that runs the Updates could be configured to run very frequently as it would not do anything if nothing had been set to update.

Project Browser use case

I know that Project Browser has been proposed at least partially as a local development tool so it that case the writable file system is probably not a problem.

But of course people will want to run it on production and I don't think it is explicitly documented that you should not do that. See related discussions 📌 Should maintenance mode be suggested or enforced? Active ?

So it would run into the same problems. I am not sure the idea to queue installs would work as well for Project Browser as people probably want immediate feedback.

Providing credentials through the form

re #18

But it's unfortunate we can't play similar tricks in Package Manger that we are in Update Manager to have "sudo" step if the server can't directly write to its own files...

I am sure many other people aren't familiar with how this works in the Update module but if people want to know more see sites/default/default.settings.php and search for "Authorized file system operations:" or if you want delve into the core `core/modules/update/update.authorize.inc`, core/authorize.php and \Drupal\Core\FileTransfer\Form\FileTransferAuthorizeForm. But we don't need to go into the details here.

Basically a user is given a form where they enter either other credentials for SFTP or SSH to transfer files of the update(or install I think).

I don't think it is impossible that we could do something like this although I don't think the API provided by \Drupal\Core\FileTransfer\FileTransfer would work for us. We might be able to us some for the current code.

But I think before we spend too much time deciding on whether could do this technically I think we figure out if we should.
I think that is product level decision:

Would it be preferable that if the web server can't write to the codebase that the user should be prompted to enter in server credentials that would allow this?

I don't think that we should assume that because this worked well for Drupal 15 years ago that we want to do this again. Unfortunately I don't think there is any way for us to know how many sites are using this part of the Update module because many sites are probably just using the Update module to get the list of available updates.

The current system in the Update module is not Composer aware so therefore it would break many sites if it installed or updated modules needed new or updated Composer dependency. So that is one reason to think it is probably not in wide usage for Drupal 8 and above. I am not sure if we can get better numbers on this. I am guessing just anecdotal

Thanks for the detailed feedback, @tedbow! A few quick thoughts for now:

1. Definitely do NOT want Drupal to ever save such credentials for later use. That would 💯 defeat the purpose of defense-in-depth here. 😂
2. I'm not necessarily proposing we need to recreate the authorize.php experience, or re-use any of that code. I'd be shocked if the "API" (such as it is) that we came up with 15 years ago would actually work for these new use cases.
3. If the run-a-command-via-cron-as-the-"super"-user approach is easy enough to include, that'd be great. For sites that care enough about security to have the files owned by a separate user, they can probably figure out how to RTFM enough to setup such a cron job. We probably don't need a UI-based version of this at all anymore. But if it's relatively easy to figure out how such a UI would work and fit in, and build all the plumbing (and counter-tops) now such that we could bolt such a sink into the house at a later time, great.
4. I'm not totally convinced that this is a no-go for Project Browser. The UI will already not be "instantaneous" -- we've gotta download things from the Internet, after all. There's already going to be a "waiting while we download (and have composer fetch all the dependencies) step. If "waiting for the download to start" takes 0-60 seconds (until the next cron job fires), that's not going to be the end of the world. I think it'd be worth building such plumbing into the PB workflow, too, and not immediately rule-out defense-in-depth with the explanation that "users will expect immediate results"... It's already not going to be immediate, the UI will have to handle that gracefully...

Gotta run, more later (I hope).

Thanks again!
-Derek

Ideally we'd have (approximate) numbers for this. We probably can't get that. But maybe we have some experience with it? Do you think it's more like 1, 10 or 50%?

I don't have data to back this up, but here's my hypothesis. From small/low-traffic sites to large/high-traffic sites, I think we have the following hosting categories:

• Generic shared hosting
• Specialty shared hosting
• Single server VM, VPS, or dedicated hosting
• Multi-server

I think the first category is the largest in terms of number of sites, and I think the vast majority of sites in this category already have a PHP-writable codebase, because the shared hosting provider gives them a single user account that's the same for ssh, sftp, and php.

For the second category, an example of specialty shared hosting might be a university with an IT department managing a pool of servers, but where each department or team can get its own website. Such an IT department might be savvy enough to provision each internal website owner with 2 user accounts: one for ssh/sftp and a different one for php.

For people running on VMs (e.g., Digital Ocean droplets), VPS, or dedicated, Drupal's docs should recommend that they set things up so that the codebase isn't writable by the web server. Even if this is a smallish percentage of total Drupal sites by number, it's an important audience, and it would be good to not steer this audience towards the less secure setup of having a webserver-writable codebase. I think this is the audience we should be thinking about for #13.

For high availability or high traffic sites that require balancing load across multiple web servers, the automatic updates module proposed for Drupal core can't be used on its own. The site would need its own deployment process to make sure the code on all web nodes is kept in sync. However, the site could setup a staging server that runs automatic updates and then triggers the site's deploy-to-prod process. Depending on the other security checks involved in that process, it might or might not be important for that staging server to have its codebase be non-writable by the web server.

@effulgentsia: That's a helpful way to categorize things in #22, thanks.

FWIW, I've been using DreamHost as my "Generic shared hosting" solution for many years. It's definitely a big player in that space. One of the key features that drew me to it in the first place was that it does let you create different users for different things, and although it takes a little extra work, you can (and I do) setup sites there so that the files are owned by a different user than httpd runs as. I'm sure I'm in the minority on that approach, but pointing out that even some of the "Generic shared hosting" solutions can still be targets for making this work (in some way) on more secure-in-depth sites.

@dww re #23 So in your case on DreamHost would the hosting also allow setup if *nix style cron jobs via Cpanel or otherwise that would also allow updating via the server cron job triggering a AutoUpdates console command(once it is built)?

Yup. You can ssh into a host and use crontab directly. I never mess with cpanel. So yes, in principle, this would all work there with the separate cron job…

Is this a nice-to-have or a must-have?

📌 Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed might be must-have (although I'm not sure why running regular Drupal cron via drush and switching off automated cron isn't enough to cover that?). I don't think any of the rest of this is. However it might be necessary to get x% extra sites to adopt automatic updates.

IMHO what @catch described would only provide an acceptable UX for Automatic Updates, not for Project Browser. Project Browser IMHO requires the ability to see the results of what you're doing at the time you're doing it, otherwise it'd be a nightmarish UX.

If I install something via the UI via Ubuntu's package manager, or via google play on my phone, I usually hit 'install', start doing something else, and then somewhere between 15 seconds to 3 minutes later get a notification that the app was installed. If I'm installing over 4g sometimes it has to give up once or twice and restart and takes a lot longer. You do get the 'percentage complete' stuff but as we all know that's a lie anyway. The fact that it happens in the background is a major feature in fact. This is also the case for manually triggered updates on those environments too. So it's a pretty standard pattern that installs and updates get queued, and eventually they get done, and then you can check back and/or get a notification that it happened. It's also IMO a better UX than 'tough, you can't use this unless you change your file permissions' and it's also a much better UX than when installs and updates used to block all other operations back in the day.

@effulgentsia:

I think this is the audience we should be thinking about for #13.

Yes this is also broadly what I have in mind - people who aren't on call to update their website on Wednesday afternoons, but can follow 'advanced' instructions step by step to get their server/Drupal configured decently. Or just people who might or might not use automatic updates, won't mind configuration it to work on their system (like adding a cron job and adding something to $settings or similar), but won't want to specifically configure their entire system to work with automatic updates.

#22: insightful analysis, thanks @effulgentsia! 😊

#26:

although I'm not sure why running regular Drupal cron via drush and switching off automated cron isn't enough to cover that?)

Agreed. But I suspect @dww's reasoning is that drush should not be required to be installed on the web server, Drupal core alone should be sufficient? @dww, can you elaborate? (I'd really rather not be guessing!)

The fact that it happens in the background is a major feature in fact. This is also the case for manually triggered updates on those environments too.

Fair! But a key difference is that Drupal does not have UI concepts in place for keeping the user informed of these kinds of things… there's never been a concept of "background tasks" and keeping the user informed of that. Although one could argue that that is what cron does, and then a Drupal status message would be the equivalent of a notification? (Easily missed/ignored though 😅)

Is that what you had in mind?

We might want to split this bit off to its own issue, but keeping going here just for now given it's currently pretty linear.

Although one could argue that that is what cron does, and then a Drupal status message would be the equivalent of a notification? (Easily missed/ignored though 😅)

Yes something like that. If we don't use queue itself, but instead key/value or similar, then we could have a constant message telling you the update/install is pending (maybe only on the project browser/automatic updates UI) and an extra message telling you it's done. We could keep something in session referencing the pending operation(s) then check status based on that, so that the logic only needs to run for the user who triggered it + maybe for everyone on the project_browser/automatic_updates UI pages in the case of multi-admin installs.

If you miss/ignore the notification, does package_manager (or project browser/automatic updates) maintain logs for what it's done? It seems useful especially for automatic updates to be able to easily see that x module was updated from x.y.y to x.y.z release (on the site rather than an e-mail), and then that could also show installs. Since updates might trigger installs too when dependencies are added, any such log probably needs to show both anyway.

I don’t exactly care about drush cron vs a dedicated script, and I’ve never raised any such concern.

I’m raising the flags that designing Package Manager to not handle the non-writable file system case is a mistake (that we can still correct) and that saying “but even if we did, it’ll never work for Project Browser because users expect immediate feedback” is misguided since nothing about Project Browser will actually be “immediate”, anyway. I’m advocating (like I have for many years) that all these update-your-site tools should work when a site is configured “correctly” 😉 to not let httpd write to its own files.

I posed the question about the Project Browser UI having a "waiting for download to start" phase, and got confirmation that:

1. The PB UI already has a progress spinner / waiting for stage X.
2. Adding another phase would be a small incremental change to the existing UI, not a whole new kind of problem for them to solve.

So I believe there's no reason to claim PB can't work with defense-in-depth, too. It's already not instantaneous, and another phase isn't a game changer for their UI. But supporting the non-writable filesystem case would be a game changer for that (subset?) of users who configure their sites "correctly". 😅

Slack transcript attached (with permission).

Thanks,
-Derek

1. @catch re #26

   📌 Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed might be must-have (although I'm not sure why running regular Drupal cron via drush and switching off automated cron isn't enough to cover that?).

   We have since removed the drush command(since core can't rely on drush and did 📌 Add Symfony Console command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed

   This allows setting up a server cron tab to run this new command as a more privileged user and keeping the codebase write protect from the user that the web server is running as

2. Setting this issue as Needs Review. The currently failing test is GenericTestExistsTest because we don't have this for package_manager. I need to figure out how to add this file to the merge request without having it be in the contrib module as it would fail in 10.1.x and 10.0.x tests

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Tests passing🎉

Gitlab tests are not passing because our tests need rsync so we will need to update .gitlab-ci/pipeline.yml to add rsync

But I think this is ready for review as the drupalci testing with rsync shows this pass if available.

Added info about the symfony console command

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The current merge request is not up-to-date with the work in the contrib module

There is automated script to convert module it but it is still a fair amount of work. For this reason and because we aren't actually getting reviews, nobody but me has commented since April, I am going stop running conversions.

If you want to review the code I would suggest reviewing the contrib module which the MR here has always been a automated conversion of. https://www.drupal.org/project/automatic_updates →

When core reviewers have time especially the product, release and framework managers and if you would like to review the module here instead of the contrib module please contact me or comment here and I can run the conversion again.

I attempted to postpone but I won't for now

MockPathLocator: This class allows tests to have a directory other than the real Drupal codebase used as the "live" directory. This lets us freely alter the "live" directory without altering the codebase that is running the test.

Without reviewing all the test coverage in detail, the first question that comes to mind here is 'why not use build tests for this?', see for example ComponentsIsolatedBuildTest - was this attempted? Is there an issue I can look at or documentation for why not? I see a couple of build tests, but they aren't using MockPathLocator which seems to be all for kernel tests.

Re #40: Package Manager has unique needs -- we need to move files around and simulate a "fake" Drupal site, but we also need to be able to test the APIs directly. Build tests are too "far" from the APIs to be useful for this; kernel tests do not, by default, deal with real files in a filesystem.

So, rather than try to get a build test to boot up a kernel in a fake site, we ended up adapting kernel tests so that they can use Package Manager's APIs with a small simulacrum of a Drupal site. That's what almost everything in PackageManagerKernelTestBase and package_manager_bypass are there to facilitate.

@catch also emphasize what @phenaproxima mentioned this is used in all of our kernel tests(also Automatic Update's)
We have 375 kernel test cases so this allows us to write the kernel tests to be simpler and more isolated than we would build test where would in a build test where even with gitlab being faster I think we would still have to worry about how long 375 build tests would take

If you wanted to look further \Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::createTestProject is where we create the test project that mock path locator will be pointed to.

In production we use \Drupal\package_manager\PathLocator as a service that tell us where the vendor directory, project root, webroot, and staging root is. We then swap this service in kernel test for the MockPathLocator

We do of course also have build test where don't do any of this mocking. Since package manager doesn't have an API we test via simple Drupal control that calls our APIs. We also have build tests Automatic Updates that test our UI, cron, automated cron, and the console command also without swapping any of services.

So, rather than try to get a build test to boot up a kernel in a fake site that isn't bootable, we ended up adapting kernel tests so that they can use Package Manager's APIs with a small simulacrum of a Drupal site based on real files. That's what almost everything in PackageManagerKernelTestBase and package_manager_bypass are there to facilitate. It's simply an easier lift than doing it the other way around.

This sounds like a good explanation - I'll try to take a closer look soon to get my head around it a bit more.

I am changing this to need review. The bot changed it to needs work in #36

The tests are not passing because we had been rely on the drupalci testing as we weren't sure in the process when core would switch to gitlab.

I think it is still reviewable all the tests pass pre-conversion in the contrib and were passing here before the switch to only gitlab

Since the switch was made we are working this 📌 Start using gitlab ci in addition to Drupalci for testing Needs work

At the moment, it's impossible to tell just from the issue summary what the remaining work is here.

🌱 Drupal 10 Core Roadmap for Automatic Updates Active is now linked (it wasn't previously), but specific issues which will result in changes to package manager prior to an alpha commit should ideally be tracked directly in this issue rather than the roadmap one I think (i.e. must haves for alpha stability, and probably any could haves?) or if necessary duplicated across the two.

Re #45, I added the links to core dependency and policy questions. It doesn't seem like 🌱 [policy, no patch] Consider whether to keep Package Manager and Automatic Updates in a separate repo/package than core in order to facilitate releasing updates to the updater Needs review but I added because it is still open and I don't think this something we would want to do later. So I think this should be closed if there consensus in core governance to do this

So this seemed to stalled, is there any help needed? Don't have the role to make any of the calls though.

@catch wonder if you have an answer or know who to ask regarding #46?

@effulgentsia just closed 🌱 [policy, no patch] Consider whether to keep Package Manager and Automatic Updates in a separate repo/package than core in order to facilitate releasing updates to the updater Needs review as 'works as designed' so I think we can call that done now.

So does that mean this should continue? Definitely needs rebasing.

I think the best next step to move this issue forward is to get reviews of the code in https://github.com/php-tuf/composer-stager. @TravisCarden just recently (2 weeks ago) released 2.0.0-beta4. It's only in beta status because it hasn't been reviewed much by people other than those working on Package Manager. As far as we know though, it's feature complete and can be marked RC or stable once it's adequately reviewed. The most noteworthy recent change is that per 🌱 [policy] Require rsync for automatic updates in Drupal core and punt other syncers to contrib RTBC we removed the PHP filesyncer so now only works if people have rsync installed. This makes the package easier to review and easier to maintain. If there's a significant population who's on systems that don't already have rsync installed and can't install it, then a separate GitHub repo/package could be created (if someone were willing to maintain it) containing alternate file syncers (e.g., one that uses the Windows robocopy command for people on Windows), but those would not be "part of core" (i.e., maintained by Drupal core committers). The Drupal issue for discussing stuff related to Composer Stager is 📌 Add php-tuf/composer-stager to core dependencies and governance — for experimental Automatic Updates & Project Browser modules Needs work . Issues and PRs could also be filed in the GitHub repo.

Beyond the above, @tedbow: it would be good to get an updated MR of Package Manager into this issue.

Ok I will update with the latest changes from the contrib module

Now that I see 📌 [PP-1] Update to Symfony 7.0 Postponed I realize that Composer Stager would have to be updated to Symfony 7. I made an issue for this https://github.com/php-tuf/composer-stager/issues/350

I assume there is no way Automatic Updates is going to get into Drupal 10.x at this point. Is that correct?

I am not sure how long the Composer Stager work will take. If it is going to take a while would it be useful for me to push up a version of this MR with the Package Manager module without Composer Stager as a dependency? All the tests would fail but it would still be latest code and could be reviewed.

https://github.com/php-tuf/composer-stager/issues/350 got completed so now Composer Stager is compatible with both Symfony 6 and 7, so I think the MR in this issue should include it so that tests run properly.

I have pushed the latest changes. The build tests will still fail in this version. I will try figure that out. It is bit difficult to get them passing in both contrib version and core version as the gitlab templates are bit different but should be doable.

This needs another rebase - conflicts in composer/Metapackage/CoreRecommended/composer.json

Did a not at all comprehensive first pass review to try to get things moving here.

Setting to Needs Review because the PR is passing and posted some responses to Catch's questions

Kicked off a pipeline to make sure tests are still green, still some unresolved feedback on the MR so moving to needs work - although reviews by more people would be really good here.

I was just reading the comments, but there are a lot here and I have not finished.

Self-assigning to fix @quietone's feedback in contrib.

On composer patches: we should not support it to start with, can open an issue to discuss introducing support for it.

Automatic updates needs to mean automatic, and that means we should not encourage/allow people to get into a situation where they have to manually hack around in composer.json/composer.patches.json in order to update their sites - because that's the direct opposite of 'automatic'.

@phenaproxima does #60 mean that @quietone's feedback has been addressed by the latest MR updates, or is that next? Should this be needs review again? What's remaining here?

@quietone's feedback has not yet been addressed; I need to sort out some process questions with @tedbow first.

xjm → credited gábor hojtsy → .

xjm → credited poker10 → .

@catch, @lauriii, @Gábor Hojtsy, @poker10, and I discussed at length how we can move this issue forward.

1. Our current focus and priority is to get this committed as an alpha module. To this end, we will do thorough reviews, but only block committing these issues on absolutely critical issues that must be fixed before the issues can be committed (e.g. issues that would have serious negative impact on contribution experience). As a reminder, with alpha-stability modules, open critical issues are not resolved before a new minor release is tagged, the modules are simply removed from the release branch.
    

2. We are aware of the downsides of making the MR the source of truth instead of the contrib module, but we've gotten to a point where this is significantly slowing contribution to the MR, so it might be time to make the MR the primary source of truth.
    

3. Blocker: 📌 Package manager/ Automatic Updates should disallow composer patches by default Active
    

4. Blocker: https://github.com/php-tuf/composer-stager/issues/78

   For this issue, the release managers think that most if not all of the remaining dev dependencies could also be removed. The original issue summary is out of date (e.g., Infection has already been removed). A contributor could help by updating this issue to reflect the current status, so that release and framework managers can make final decisions about which dev dependencies to remove.
    

5. Blocker: 📌 Add php-tuf/composer-stager to core dependencies and governance — for experimental Automatic Updates & Project Browser modules Needs work (This is blocked on the core MR above.)
    

6. Blocker: 📌 Package manager/ Automatic Updates should disallow composer patches by default Active
    

7. Non-alpha-blocker: https://github.com/php-tuf/composer-stager/issues/300 (This can be treated as a docs gate stable blocker.)
    

8. Non-alpha blocker: The current performance issues with the server-side implementation can be treated as beta- or stable-blockers, but do not need to block an alpha commit.
    

9. The issue is a priority across the board as a blocker for basically every other important thing we're doing (including Starshot), so we want to try to focus resources on it. To that end:
    

10. Lauri will speak to Dries about adding a section to the Driesnote about this.
     

11. Separate issue needed: Contact the DA about issue credit bonuses for this and its related blocking issues (probably as well as followups through AU-stable.)
     

12. We could accelerate work with faster review cycles, so coordinating our work such that the technical leads have time to respond when the FM and RMs provide reviews would also help us make faster progress.

Hopefully I didn't miss or misrepresent anything. 🤞 Thanks everyone for your dedication on this issue!

I opened 📌 Package manager/ Automatic Updates should disallow composer patches by default Active but I don't think it needs to block an alpha commit of package manager, I just think we need to decide exactly what the expectations and behaviour are (then change them if necessary) before automatic updates is beta, so the issue being open is enough for me on that one.

I also think we can open a follow-up for the API/terminology questions on how 'stage' is used - it might involve small API changes if a class/method gets renamed but that's OK during alpha. Would be good to clarify before stable though because it's a bit confusing what's what at the moment at least to me. It's a couple of months since I actually read the code so not sure I could write a coherent issue summary about it right now.

Re: #66

4. Blocker: Evaluate dev dependencies · Issue #78 · php-tuf/composer-stager

For this issue, the release managers think that most if not all of the remaining dev dependencies could also be removed. The original issue summary is out of date (e.g., Infection has already been removed). A contributor could help by updating this issue to reflect the current status, so that release and framework managers can make final decisions about which dev dependencies to remove.

I've updated the issue summary there.

Opened 📌 Clarify/replace 'stage' terminology in Package Manager Active .

re #66
@xjm and @catch, @lauriii, @Gábor Hojtsy, @poker10

This sounds great!

We are aware of the downsides of making the MR the source of truth instead of the contrib module, but we've gotten to a point where this is significantly slowing contribution to the MR, so it might be time to make the MR the primary source of truth.

That sounds like a good idea. The contrib module has not needed much work in a while. The usage for the 3.x branches has been slowing been creeping up and we haven't been getting bug reports 👍

I just changed the contrib project to "Maintenance fixes only" because we are not adding any new features.

I talked to @phenaproxima and we are not going push the changes on this MR back to the contrib project, just to save time. But if we find a serious bug that say stopped a site from getting unattended updates we would address that.

As you can already see @phenaproxima is taking the lead on addressing the MR here, thanks for that.

Very excited to see this moving forward! Thanks everyone!

@tedbow @phenaproxima does this mean the disclaimer/notice at the top of the issue summary can be removed? It would be great if people could make MR suggestions and/or possibly commits for small changes without feeling like they were going to break a workflow.

I haven't done an end-to-end review here since August but I will try to do that again soon.

@catch thanks for the reminder, removed

OK, I think feedback is largely if not totally fixed now. I also made a few modernization changes (mostly adopting service autoconfiguration).

I've gone through and marked all but three of the threads opened by me and @quietone resolved - either the change was made, or there was a good answer for why not, or somewhere in-between.

That leaves three threads unresolved which are mostly minor-ish follow-up issues and one question about whether a follow-up issue/@todo is needed.

I'm going to try to go over this one more time, ideally this week, now that all the nits are dealt with - this should not stop anyone else reviewing it.

I just re-reviewed the entire MR except for the test fixtures and test coverage. Did not find much, mostly nits, there is one comment near the end where I'm concerned about a race condition if the same user manages to try to acquire a lock twice (e.g. in two different browser tabs).

I don't think any of these are blocking but it would be good to quick fix the ones that can be or open follow-ups if not.

I also opened 📌 Document results of security audit Active which does seem important.

Notwithstanding the above this looks ready to me otherwise.

I read through the MR bottom to top up to where I left off last time:

1. Some minor comments on hook_help() and hook_requirements(). I think we should try to slim down the hook_help() in a follow-up, probably moving the FAQ/trouble shooting steps to d.o

2. I read through about 80% of the test coverage before running out of steam. I can't pretend I reviewed all the logic in the tests however:

1. There is a good mix of unit, kernel, build etc. tests. I imagine we'll get more high level coverage via auto updates + project browser too.
2. There's plenty of test coverage, and a lot of the logic is in traits which should make it easy to add more coverage when needed.
3. Tests are pretty good about testing the right amount of stuff so easy enough to follow.

Needs a rebase, looks like only for composer.json/lock conflicts, and a couple more follow-ups opened, but otherwise I'd be happy to RTBC this once those are done.

One thing slightly bothers me that this adds an event-based validation API, when we have Symfony validator in core alread - but.. I don't know what it would begin to look like to use the Symfony validation API here - this needs self-registration and priorities etc. I also would personally climb the walls if we had to split all those validators into two classes to have a validator vs. constraint. So... don't think it bothers me after all.

I'm removing the release manager and framework manager tags from this because I've done plenty of review from both aspects over the past 18 months. If I RTBC it, that means I won't be able to commit it, so another committer will have to do that anyway.

Needs work for those last bits. Should not stop anyone else reviewing though.

Thanks all the new changes look fine as do the two follow-ups, feel happy marking this RTBC now!

Also 'normal' feels like underplaying how important this is. Not keen on 'critical feature requests' so splitting the difference at major.

We discussed this with Lauri yesterday as Drupal core product managers. We have no concerns. The added code does not have UI parts other than some error messages which seemed fine. Two modules already integrate well with it and we agree the product needs it for both. Being an API module, we accept the judgement of the framework managers on the APIs.

The issue summary says please do not push to the branch but the way the MR is changing the dictionary is incorrect and there are whitespace errors in core/modules/package_manager/tests/fixtures/fake_site/README.md - no idea how to contribute these back properly.

The issue summary says please do not push to the branch

That's outdated now the MR is canonical as off a month or so ago.

Discussed with @catch - we need a change record and a release note because we're moving a module in from contrib.

I've had a look at much of the code and am happy to commit this once those exist.

Discussed a bit more with @catch. I realised that there's nothing for anyone to do until we've done a few things and the release note and CR can only happen once package_manager is stable in core. This is because the package_manager module in contrib is part of https://www.drupal.org/project/automatic_updates → - it’s not a separate module. So I think we’re going to need new releases of the contrib module. We will need a new major to remove the package_manager sub module and a new patch for the existing major/minor to declare a conflict with which ever version of core ships with a stable package manager.

Assuming we're able to get automatic updates in (renamed) to core, and package_manager and automatic updates reach stability in the same minor release, it might just be that the contrib module either doesn't get any update, or eventually gets an update to uninstall itself and install the renamed automatic updates version, and maybe removing itself from composer if it's able to do that. Either way it means there's nothing to do for this issue specifically in terms of release notes / CR and we can sort that out when it's all a bit closer.

Crediting all the people who contributed to the package_manager module in contrib

alexpott → credited abhishek_gupta1 → .

alexpott → credited ajits → .

alexpott → credited anish.a → .

alexpott → credited bnjmnm → .

alexpott → credited capysara → .

alexpott → credited chrisfromredfin → .

alexpott → credited cola → .

alexpott → credited daisyleroy → .

alexpott → credited diegors → .

alexpott → credited drumm → .

alexpott → credited fizcs3 → .

alexpott → credited grasmash → .

alexpott → credited heddn → .

alexpott → credited Idoni → .

alexpott → credited immaculatexavier → .

alexpott → credited Ishani Patel → .

alexpott → credited jayesh.d → .

alexpott → credited kjankowski → .

alexpott → credited kunal.sachdev → .

alexpott → credited leksat → .

alexpott → credited lhridley → .

alexpott → credited narendra.rajwar27 → .

alexpott → credited narendrar → .

alexpott → credited omkar.podey → .

alexpott → credited p.ayekumi → .

alexpott → credited percoction → .

alexpott → credited rahul_ → .

alexpott → credited Ranjit1032002 → .

alexpott → credited rkoller → .

alexpott → credited rocketeerbkw → .

alexpott → credited Schnitzel → .

alexpott → credited shabbir → .

alexpott → credited siramsay → .

alexpott → credited srishtiiee → .

alexpott → credited s_leu → .

alexpott → credited Theresa.Grannum → .

alexpott → credited tim.plunkett → .

alexpott → credited Webbeh → .

alexpott → credited wendyZ → .

alexpott → credited wiifm → .

alexpott → credited yash.rode → .

Crediting myself and @xjm for work on this issue.

Committed 2975694 and pushed to 11.x. Thanks!

Opened 📌 Update to stable php-tuf releases Active .

Great to see this in.

This commit added build tests in a module, and currently phpunit.xml.dist is not listing them in the suites configuration.

🐛 Ensure run-tests.sh and PHPUnit CLI run with the same list of tests to be executed Active will fix that.

Should there be a change notice for this? Or maybe an 11.1 tag?

As an alpha experimental module it is likely to be removed from the 11.1 release and only remain in the 11.x branch until more of Automatic Updates or Project Browser are ready to be committed and tested. It was committed here to unblock reviews and work on those projects.

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