Problem/Motivation

Should not be merged till Installer is merged.

Follow up to #3245770: Create a service to composer install via package_manager from Automatic Updates

When doing a Composer require of a new module it is possible the module will require a newer version of core.

Proposed resolution

Implement a listener to the PreApplyEvent to throw an error if the Drupal core version changed. This should not be allowed.

Remaining tasks

  • ✅ File an issue about this project
  • ☐ Addition/Change/Update/Fix to this project
  • ☐ Testing to ensure no regression
  • ☐ Automated unit/functional testing coverage
  • ☐ Developer Documentation support on feature change/addition
  • ☐ User Guide Documentation support on feature change/addition
  • ☐ Code review from 1 Drupal core team member
  • ☐ Full testing and approval
  • ☐ Credit contributors
  • ☐ Review with the product owner
  • ☐ Release

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork project_browser-3249550

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Parent issue: #3249549: Warn user if other modules were added or updated before install »
Related issues: +#3245770: Create a service to composer install via package_manager from Automatic Updates

yash.rode made their first commit to this issue’s fork.

yash.rode’s picture

yash.rode commented
Assigned: Unassigned » yash.rode

yash.rode opened merge request !210

yash.rode’s picture

yash.rode commented
Assigned: yash.rode » Unassigned
Status: Active » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

When doing a Composer require of a new module it is possible the module will require a newer version of core.

🤔 I question this.

Is it really that big of a problem if a module installation causes a patch-level core update? Updating to a different minor (or major!) I can understand...but blocking patch-level updates seems a little draconian and user-unfriendly to me. I'd like to get @tedbow's thoughts here.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

IMHO we should postpone this on #3245770: Create a service to composer install via package_manager from Automatic Updates, because right now we're cross-pollinating these two merge requests.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

re #7 I think should be up to the Project Browser team but as that made Package Manager we should help them make an informed decision.

Problems I see with allowing core updates

  1. Even patch level Core updates could have DB updates that need to be run. Depending on Project Browser's validation around other dependencies updates(will they allow other drupal projects to be updated during an install?) these might be the only reason to put this logic in
  2. The patch level update may be a security release, even a highly critical one, where the site owner should read the core release notes. Even if we provide a link to that page are the users who are given access to install new modules via PB going to be the same users that can access the the core release notes in such a situation.
  3. site admins may choose to turn on only PB and not AutoUpdates because they maybe have a specific processes such as QA and testing around updating core.
  4. This would increase the scope of #3284945: Install endpoints that leverage Package Manager + core APIs because it would have handle the case where a core update happens during an install and inform the user in responsible way

My advice would be not to allow any core updates until the implications are throughly researched and assessed. These are only the problems I could think of off the top of my head. I think there are probably other consideration.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

OK, makes sense. Let's maybe open a follow-up issue, with a @todo in the doc comment of the class, to re-evaluate this and revisit the need for it.

yash.rode’s picture

yash.rode commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work
yash.rode’s picture

yash.rode commented
Status: Needs work » Needs review

aarti zikre made their first commit to this issue’s fork.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work

Sending back to "needs work" for the test failures to be resolved.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

So, about the weird failure, @bnjmnm had this to say:

I was getting that exact error last month and addressed it by adding an explicit dependency to Project Browser in project_browser_devel.info.yml Try doing that in `project_browser_test.info.yml . I remember there being a reasonably sane explanation when I figured out that was the solution but I can't recall the specifics so I'm not 100% that will fix it for you, but it's easy enough to try

So try that, I guess, and see if it helps!

yash.rode’s picture

yash.rode commented
Status: Needs work » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: Create package_manager validator to ensure core was not updated during a project install » [PP-1] Create a validator to ensure core was not updated during a project install
Status: Needs review » Postponed

Looks good to me. Consider this an RTBC, once the blocker is merged. Nice work, @yash.rode!

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Postponed » Needs work

#3245770: Create a service to composer install via package_manager from Automatic Updates is committed! We should remove any duplicate code here

bnjmnm made their first commit to this issue’s fork.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Title: [PP-1] Create a validator to ensure core was not updated during a project install » Create a validator to ensure core was not updated during a project install

Rebased and available for someone to resume work on it.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs work » Needs review
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work

For MR comments

yash.rode’s picture

yash.rode commented
Status: Needs work » Needs review
yash.rode’s picture

yash.rode commented
Assigned: Unassigned » yash.rode
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work

getting close!

yash.rode’s picture

yash.rode commented
Status: Needs work » Needs review
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

@yash.rode thanks everything looks good now!

tim.plunkett made their first commit to this issue’s fork.

narendraR made their first commit to this issue’s fork.

yash.rode’s picture

yash.rode commented
Assigned: yash.rode » Unassigned
narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Issue tags: +Package Manager integration in Project Browser
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Needs work

Rebased right up until #3306722: Update Installer service to work without requiring to specify the package version, that will need rebase from someone who knows the MR better.

yash.rode’s picture

yash.rode commented
Status: Needs work » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I don't see a real problem here. I'm not sure about the phrasing of the validation message, but that could be fixed later.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented

Done some changes in test after https://www.drupal.org/project/project_browser/issues/3310703 is merged.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs review » Reviewed & tested by the community

All feedback addressed and changes seems good to me. Marking as RTBC.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Rebased, fixed a few nitpicks, now merging. Thanks for sticking with this @yash.rode!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Fixed

Merged!

Status: Fixed » Closed (fixed)

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