Problem/Motivation

8.8.0 moved path aliases to entities in #2336597: Convert path aliases to full featured entities
However pathauto module requires updates accordingly - see #3012050: Prepare for the conversion of path aliases to entities
We should try to prevent people updating to 8.8.0 if they don't also update pathauto. We can do this for composer users. Not sure we can do anything for drush/tarball users - short of a hook_requirements - but I don't think we want to put a contrib module specific hook_requirements in core.

Proposed resolution

Add a conflict entry to composer.json on pathauto versions 8.x-1.4 and below

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Drupal 8.8 requires pathauto versions of 8.x-1.6 or higher (if installed). You should upgrade pathauto first, then update to Drupal 8.8

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan created an issue. See original summary.

larowlan’s picture

Status: Active » Needs review
FileSize
392 bytes
jibran’s picture

See #3012050-15: Prepare for the conversion of path aliases to entities

What I was wondering about is whether we could make a patch that works with both 8.7 and 8.8, that would mean we would have to add these methods back, and we could possibly even have two different implementations for 8.7 and 8.8 that we could register dynamically. That should work quite well. The generator and some other parts might be uglier?

While more work, I think that might also be beneficial for the being able to get the core issue in and we could commit this early, making testing and updating easier?

Path auto might support both core versions at the same time.

larowlan’s picture

Path auto might support both core versions at the same time.

Right, but it can't do that retrospectively, you won't get those features without updating to the newer version of pathauto, which is what this patch makes you do. e.g. with this patch you can't update to 8.8.0 unless you update to pathauto 1.5+ at the same time.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

you can't update to 8.8.0 unless you update to pathauto 1.5+ at the same time.

This makes sense to me so setting it to RTBC.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Upgrade path

I think we should actually add hook_requirements(); we've done this in the past when contrib modules have a conflict with a particular version of core, e.g. when we were first adding Media to core and there were conflicts with the contrib Media Entity API. In that situation we added hook_requirements() to both core and the contrib module that prevented you from installing or updating them together.

xjm’s picture

This should also go in the release notes (or be added to the existing release note about the data model change and pathauto).

catch’s picture

+1 on adding the hook_requirements(). We can have a 9.x follow-up to remove it again (probably need to keep it in for the remaining lifetime of 8.x though).

jibran’s picture

Assigned: Unassigned » jibran

Working on it.

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
FileSize
1022 bytes
1.38 KB
+++ b/core/composer.json
@@ -49,7 +49,8 @@
-        "drush/drush": "<8.1.10"
+        "drush/drush": "<8.1.10",
+        "drupal/pathauto": "<1.5"

I'm not sure that we need to update composer.lock file for this or not. I ran composer update --lock and it did nothing.

alexpott’s picture

@jibran if you do $ composer update drupal/core then the lock file gets updated properly. There has been a few core/composer.json updates where the lock file has not been updated correctly.

jibran’s picture

FileSize
2.55 KB
3.44 KB

Thanks, @alexpott. Here is the new patch. Any suggestion to improve the Englizh in hook_requirements?

Berdir’s picture

Note: I just discussed with @amateescu that I plan to do another release *before* we make it 8.8 compatible.

I had planned to then in that release make it incompatible with 8.8, so people who update to that until 8.8.0 comes out are covered (assuming that works in .info.yml, will check).

The problem is that this hardcodes 1.5 and would need to be updated then, and we might even need to do another release until then.

I would suggest to hold off on this until I've done the 1.5 release at least and then decide if this is really necessary. This is not an alpha blocker IMHO.

Berdir’s picture

AFAIK, the public testing program starts with beta, correct? Because it obviously wouldn't be that useful then before a compatible pathauto release is out, because which site doesn't use that? ;)

larowlan’s picture

Status: Needs review » Postponed

Makes sense, thanks @Berdir

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

Status: Postponed » Needs review

OK I was wondering how it was I didn't have the release note for this -- now I see why.

Despite that alphas are only recommended for developers, we generally want to mitigate any data loss issue in any tagged release, insofar as it is possible.

What's the status of pathauto releases this week?

jibran’s picture

amateescu’s picture

@xjm, we're waiting with the patch that prepares Pathauto for 8.8 to see if #2233595: Deprecate the custom path alias storage can land before the next alpha or beta :)

Berdir’s picture

I've now released 8.x-1.6-alpha1 that is compatible with Drupal 8.8-alpha1 *and* Drupal 8.7. And I'll probably release that for beta or so. Not sure if you still want to do this, no strong opinion. version description imho says pretty clearly what is supported for 1.5 and 1.6

catch’s picture

Status: Needs review » Needs work

There's some minor composer.lock cruft in the patch, but also based on Berdir's comment we should declare the conflict on < 1.6

I think this is still worth doing even though sites can update to 1.6 within Drupal 8.7 since they still might not do that before updating core.

jibran’s picture

Status: Needs work » Needs review
FileSize
1021 bytes

Updated the patch.

catch’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs release note

This is missing the system_requirements() hunk from #12, but otherwise looks good.

Added a release notes snippet.

jibran’s picture

Uploaded the interdiff instead of patch. :D Here we go again.

jibran’s picture

+++ b/core/modules/system/system.install
@@ -1095,6 +1095,22 @@ function system_requirements($phase) {
+      if (version_compare($info['version'], '8.x-1.5') < 0) {

As per https://www.drupal.org/project/pathauto/releases/8.x-1.6-alpha1

version_compare('8.x-1.6-alpha1','8.x-1.5');
=> 1

is fine.

+++ b/core/modules/system/system.install
@@ -1095,6 +1095,22 @@ function system_requirements($phase) {
+          'description' => t('The Pathauto module is not compatible with Drupal core version :version. Please update Pathauto to 8.x-1.5 or later.', [

But we do need to update.

jibran’s picture

amateescu’s picture

version_compare($info['version'], '8.x-1.5') < 0 doesn't catch the case when the Pathauto version is 8.x-1.5. These are all the cases that we need to take into account:

version_compare('8.x-1.4', '8.x-1.5');
=> -1
version_compare('8.x-1.5', '8.x-1.5');
=> 0
version_compare('8.x-1.6-alpha1', '8.x-1.5');
=> 1
version_compare('8.x-1.6', '8.x-1.5');
=> 1

So we need to change the comparison to <=. Also tweaked the wording a bit and changed the call to the deprecated system_get_info() function.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, it seems legit to me. There is nothing technical in the patch other than what @amateescu explained so setting it RTBC also @catch gave 'looks good' in #23.

  • catch committed e2258e2 on 9.0.x
    Issue #3085062 by jibran, amateescu, larowlan, xjm, Berdir: Declare a...

  • catch committed 3aaadd7 on 8.9.x
    Issue #3085062 by jibran, amateescu, larowlan, xjm, Berdir: Declare a...

  • catch committed 0e20ceb on 8.8.x
    Issue #3085062 by jibran, amateescu, larowlan, xjm, Berdir: Declare a...
catch’s picture

Version: 8.9.x-dev » 8.8.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Added a change record and committed/pushed to 9.0.x/8.9.x/8.8.x, thanks!

Status: Fixed » Closed (fixed)

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

Krzysztof Domański’s picture

I publised Drupal 8.8.0 requires pathauto version 8.x-1.6 or higher if installed and I added to CR:

"Pathauto 8.x-1.6 is compatible with Drupal 8.7 and 8.8. You must update to the latest version of Pathauto before you update to Drupal 8.8.0 or at the same time."

I think the release notes is confusing. It say "you must update to the latest version of Pathauto before you update to Drupal 8.8.0.". It is possible to update both together.