Closed (fixed)
Project:
Project Browser
Version:
1.0.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
26 Jul 2022 at 18:09 UTC
Updated:
2 Mar 2023 at 20:20 UTC
Jump to comment: Most recent
Problem/Motivation
#3284945: Install endpoints that leverage Package Manager + core APIs will expose installing via Composer through the UI
#3245770: Create a service to composer install via package_manager from Automatic Updates was split out to just handle the package_manager(the sub-module of AutoUpdates) integration
But regardless of the code the rules for Composer operations should be well defined. Asking Composer to require a new package could result in other side effects. Although there will be validation through the UI the installer service should not perform Composer operations that do not follow the rules that will be defined.
Example of possible side effects of requiring a new project
- Vendor dependencies could be update
- New vendor dependencies could be added
- Core could be updated to any version
- Another Drupal project on site could be updated, this project may or may not have extensions that are currently installed(in the Drupal meaning)
- Another Drupal project could be installed that is currently not in the code base
- Custom Drupal projects could be added or updated. these projects would not have drupal.org Update XML
- Any of the Drupal projects that were updated could have database updates. that will need to be run. This includes core and any enable extensions that are enabled
- Drupal projects that were update or installed could be on insecure or unsupported versions according to drupal.org's Update XML
- If the requested project's version is not explicitly set then it could be installed to a insecure or supported version
- A Drupal project dependency could be updated or installed via Composer but the project may have already been installed on the site just not through Composer.
We should not assume any of these things will not happen based on the Composer command that is run. Package Manager provides a PreApply event where the actual staged packages can be check against the active code base.
Proposed resolution
Side affects Project Browser will allow in the MVP version of Composer installs
Allow all new/updated vendor packages and new/updated Drupal modules and themes.
Only Drupal modules and themes are that secure according to drupal.org Update XML will be allowed to have there version affected during an update. Only updates and never downgrades to Drupal projects will be allowed. This would mean that Project Browser would responsible for warning the user about database updates and making sure the database updates are run after the update.
Operations that would would conflict with extensions not known to Composer will cause an error. This will forbidden operations if
- A new project is installed but that project is already in the codebase *anywhere* but is not known to Composer
- A new project is installed and adds a new dependency that is already in the codebase *anywhere* but is the dependency not known to Composer
- A new project is installed and updates an existing dependency but is the dependency not known to Composer
All of the conditions would only trigger an error after the Composer operation the is staged. So the user would have to attempt to do the update first and then be notified of the error. This is because we can't fully know what dependencies would be updated or added before operation is executed. The staged composer operation will not affect the active site.
The individual issues needed for the above validation are laid out in #3300309: [Meta] Use Package Manger(From AutoUpdates) API to install via Composer. This issue is just to get agreement on the above validation/restrictions.
#3284945: Install endpoints that leverage Package Manager + core APIs should not be committed until validation enforces the desired behavior.
Remaining tasks
Sign-off
Comments
Comment #2
tedbowComment #3
fjgarlin commentedIt seems like a lot of things could happen if we’re not careful.
What’s the approach that autoupdates takes when updating existing packages?
What’s the default behaviour for package_manager when requiring a new package if we don’t specify anything?
I think a good approach could be to check if the module can be required cleanly without updating existing Drupal modules (ie: requiring module A will not mean updating module B and/or core). I think that "updating" should fall in the realm of #autoupdates, whereas all we want in project_browser is to bring the code to the codebase and enable it.
So we could do the check (not sure how), and if not possible, just say it to the user. ie: if requiring "search_api_solr" requires "search_api:>3.2" but we have "search_api:3.1", then just tell the user that's not possible to bring the module unless they update "search_api" by themselves. I don't think that project_browser should decide how to continue, whether to update other modules or not, etc.
Just my opinion, happy to hear other people's thoughts.
Comment #4
bnjmnmI'd like there to be three response categories of side effects:
Including "Requires Confirmation" logic adds a bit of dev overhead, and we'd have to discuss which scenarios fall into which category. I think it's important to have this to keep Project Browser feeling useful. If we send people to Composer too often, especially for reasonably safe scenarios, it's going to discourage users.
The confirmation step also provides us with the opportunity to re-phrase Composer language to something Drupal-specific that is potentially easier to understand than working directly via CLI.
Comment #5
tedbowComment #6
bnjmnmAlthough I ultimately want the categories as mentioned in #3 to distinguish between warning and error, I'm also fine with #3284945: Install endpoints that leverage Package Manager + core APIs landing with everything being install-preventing errors. This helps contain the scope of that issue, and the warning/confirmation-modal functionality can be done in a followup. Ideally that followup is completed before there's a release that introduces UI installs, but even if there isn't the feature addition would be an overall improvement.
Comment #7
tedbowI am also looking at what validation should be added to Package Manager. For instance #3303124: [meta] Package Manager should add validation to try avoid conflicts with non-Composer installed code, could also affect Project Browser because a project could installed that is not known to Composer but was already installed in a non-Composer way on the Drupal site, which could result in duplicate extensions.
Comment #8
fjgarlin commentedProject Browser checks which modules are present using Drupal functions. getting the module list information from Drupal, regardless of where that module was installed from.
If the module is there already, PB would only offer the possibility of "install" module, which should probably use Drupal APIs for it too.
The list of modules is passed to the front-end here: https://git.drupalcode.org/project/project_browser/-/blob/1.0.x/sveltejs...
From the back-end here:https://git.drupalcode.org/project/project_browser/-/blob/1.0.x/src/Cont... and https://git.drupalcode.org/project/project_browser/-/blob/1.0.x/src/Cont...
The only thing I'm not sure about is if package_manager takes care of installing the module. If that's the case, then yeah, we need to check that the module is there or not already, but from PB perspective, the actions offered will be based on whether the module is there or not.
Comment #9
tedbow@fjgarlin thanks for the links to the relevant code.
Yes package_manager would install the module but it would not be aware of Project Browser specific limitations.
Since #3245770: Create a service to composer install via package_manager from Automatic Updates created a new service to Composer require projects via Package Manager that service should not make any assumptions about any project filtering that was done before the service was called. It should impossible to install projects via this service that don't pass the validation that we establish in this issue.
One reason for this is to protect against edges cases where 2 users are trying to install the same module at the same time via Project Browser(or any other future module that uses Package Manager) and 1 user leaves their project listing page open while another installs the same module. Or really if a 1 user is trying to install Module A and another user then installs Module B which has module A as a dependency.
Also 1 user could be installing through project browser while another is installing on the command line through Composer.
Comment #10
tedbowAfter talking with @tim.plunkett I added a 4th option to the proposed changes that would allow updated modules with DB updates. It would be Project Browser's responsibility to
Comment #11
phenaproximaFor whatever my opinion is worth, I think Project Browser should be as lenient as possible. The baseline, IMHO, is that it should never allow installing insecure versions of anything. (That will likely require a dependency on the Update module, which might in turn flow down into Package Manager.) Beyond that, I'd say it should allow people to install basically whatever they want, as long as Composer's willing to do it.
Comment #12
tedbowComment #13
tedbowI updated the proposed solution.
Comment #14
tedbowadded related Package Manager issues and updated summary
Comment #15
narendrarComment #16
tedbowI have update the summary with the proposed restrictions. The road for how to get this done is #3300309: [Meta] Use Package Manger(From AutoUpdates) API to install via Composer
I have RTBC'ed this issue because I think there is agreement on the validation/restrictions I have proposed. But someone from PB should sign-off
Comment #17
tim.plunkettI'm +1 on this, assigning to Chris for sign-off.
Comment #18
chrisfromredfinSo while I agree with everything in the policy laid out, I think it might be incomplete if we don't consider a confirmation step like @fjgarlin and @bnjmnm have laid out; however, I'm unsure if that's a separate issue for Project Browser's UI to handle, or for this issue.
I'm looking specifically at side-effect 3 - "core could be updated to any version."
If that's really the case, we must be able to offer users a way to cancel out, if, for example, it might take them across a major (or maybe even a minor) version of Drupal core.
I envision it as, after staging, it tells the user "the following updates will be applied: drupal/core 9.4.6 -> drupal/core 10.0.7, token 2.3.1 -> token 3.1.2"
...then the user has the opportunity to say "hey, wait, nevermind - not ready to go to Drupal 10 yet."
HOWEVER, I think in terms of pure validation, this is sufficient. That is, I don't think we want to preclude package_manager from allowing you to opt up in core, or any other package... it just needs a confirmation in the UI between the staged version and applying it.
If I understand this (above) correctly, then I'm +1 also.
Comment #19
chrisfromredfinAfter speaking with Ted directly today at DrupalCon, I think a Project Browser-based policy to not update core is the right thing altogether, especially in initial/MVP stages. We want to minimize giving people a broken site, so preventing upgrades to core seems like the right thing, and it can always be changed if we end up seeing/needing it a lot. But I think it's a minimal enough use case to not worry.
Comment #20
chrisfromredfinComfortable marking this fixed now since documentation child issue opened here: https://www.drupal.org/project/project_browser/issues/3311016
Comment #22
chrisfromredfin