Should have used the entity type manager :)

CommentFileSizeAuthor
#6 3093401-6.patch6.03 KBjibran
#6 interdiff-cd76b7.txt1.07 KBjibran
#5 path_alias-3093401-5-interdiff.txt552 bytesBerdir
#5 path_alias-3093401-5.patch6.1 KBBerdir
#4 path_alias-3093401-4-interdiff.txt3.09 KBBerdir
#3 interdiff.txt2.95 KBslasher13
#4 path_alias-3093401-4.patch5.56 KBBerdir
#2 path_alias-3093401-2.patch3.63 KBBerdir
#3 path_alias-3093401-3.patch5.59 KBslasher13
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Active » Needs review
FileSize
path_alias-3093401-2.patch3.63 KB

Using the alias repository is 8.8.0-beta1+, but using the entity type id instead of class would have been forward compatible :)

Not 100% sure that will work with the BC layer, lets see.

slasher13’s picture

slasher13
Primary language German
CreditAttribution: slasher13 as a volunteer and at Wolters Kluwer commented
FileSize
interdiff.txt2.95 KB
path_alias-3093401-3.patch5.59 KB
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
FileSize
path_alias-3093401-4.patch5.56 KB
path_alias-3093401-4-interdiff.txt3.09 KB 
  1. +++ b/pathauto.info.yml
@@ -6,7 +6,7 @@ type: module
 - drupal:path
-- drupal:system (>=8.6)

-- drupal:system (>=8.6)
+- drupal:system (>=8.8)
 - token:token

    this is not necessary, you can see that my patch still passed on 8.7. We have a service provider in place that alters out the implementation with a 8.7-compatible version. 

  2. +++ b/pathauto.services.yml
@@ -7,12 +7,12 @@ services:
   pathauto.alias_uniquifier:
     class: Drupal\pathauto\AliasUniquifier
-    arguments: ['@config.factory', '@pathauto.alias_storage_helper','@module_handler', '@router.route_provider', '@path.alias_manager']
+    arguments: ['@config.factory', '@pathauto.alias_storage_helper','@module_handler', '@router.route_provider', '@path_alias.manager']
   pathauto.verbose_messenger:
     class: Drupal\pathauto\VerboseMessenger

    That also means we can't change this, because we need to keep using the deprecated service to be compatible with 8.7.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
FileSize
path_alias-3093401-5.patch6.1 KB
path_alias-3093401-5-interdiff.txt552 bytes

> this is not necessary, you can see that my patch still passed on 8.7. We have a service provider in place that alters out the implementation with a 8.7-compatible version.

A BC layer that we need to update now that the arguments for the new service are actually fixed :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext for Service NSW commented
FileSize
interdiff-cd76b7.txt1.07 KB
3093401-6.patch6.03 KB 
+++ b/pathauto.services.yml
@@ -7,7 +7,7 @@ services:
+    arguments: ['@config.factory', '@path_alias.repository', '@database','@pathauto.verbose_messenger', '@string_translation', '@entity_type.manager']

+++ b/src/AliasStorageHelper.php
@@ -7,10 +7,9 @@ use Drupal\Core\Database\Connection;
-use Drupal\Core\Path\AliasStorageInterface;
...
+use Drupal\path_alias\AliasRepositoryInterface;

'@path_alias.repository' service implements \Drupal\Core\Path\AliasRepositoryInterface not \Drupal\path_alias\AliasRepositoryInterface so we have to revert this change.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

No, it doesn't, that's just the BC layer in core.services.yml that's confusing phpstorm. This interface is deprecated and the service is changed in \Drupal\path_alias\PathAliasServiceProvider.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext for Service NSW commented

No. it was not phpstorm it was me. I missed \Drupal\path_alias\PathAliasServiceProvider :) your patch is correct but as described in chat it doesn't work with `drush updb` which updates the service definition before installing path_alias.

slasher13’s picture

slasher13
Primary language German
CreditAttribution: slasher13 as a volunteer and at Wolters Kluwer commented
Status: Needs review » Needs work

Tested patch #5 and i got:

TypeError: Argument 2 passed to Drupal\pathauto\AliasStorageHelper::__construct() must be an instance of Drupal\path_alias\AliasRepositoryInterface, instance of Drupal\Core\Path\AliasRepository given, called
in \core\lib\Drupal\Component\DependencyInjection\Container.php on line 289 
in Drupal\pathauto\AliasStorageHelper->__construct()
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs work » Fixed

@slasher13: Let me guess, you also use drush9? We identified that as the problem here, because it instantiates services too early. But since we can't control that, I'm OK with committing #6 even though it will result in extra work down the line.

I've also updated the property @var type and committed this.

  • Berdir committed 7bc985b on 8.x-1.x authored by slasher13 
    Issue #3093401 by Berdir, slasher13, jibran: Compatibility with Drupal 8...

Status: Fixed » Closed (fixed)

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

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

FYI I've ran into this problem upgrading a site from 8.5.3 to 8.9.0 (and two+ years of contrib module updates) when using Drush 8 to run updatedb.

laurent.lacote’s picture

laurent.lacote CreditAttribution: laurent.lacote commented

Might as well report it here. I also ran in a similar problem.

Tried to upgrade everything at once (notably core from 8.7.8 to 8.9 straight), with composer, having already pathauto 1.6
Turns out the backward compatibility is ONLY IN 1.6 EXACTLY.
So because I didn't hardlock version on pathauto (why would have I?) it went up straight to 1.8.

Thus turning what should be a simple operation into an vicious circle hell, because you cannot run updatedb because service is missing (logically); but you cannot either uninstall pathauto just long enough to do update, since, you know, drush crashes also here because service missing.
In fact, you're basically screwed. HARD. Website interface dead, drush breaking on any meaningful operation, only salvation is trying to hack into either the code or the database.

Personally, I find this a kinda bad practice to ensure backwards compatibility for a very limited time and not even being clear about it.
There really should be a strong and clear mention on the project page, for example in the "Known Issues" section, that if you are still in core <= 8.7, you NEED to lock yourself into 1.6 EXACTLY until you got the core in 8.8 or higher.

I'm sorry that I come at this strongly, but having found the explanation only when I tried to downgrade pathauto until I found "a version that worked" after spending a dozen hours trying to get myself out with drush and composer clearly didn't help my humor. :)
You just cannot expect everyone to be as knowledgeable as you with all those technologies, and (as much as we can all regret it ^^), you cannot expect everyone to closely follow new versions as they are published either.

With that rant aside, I sincerely thank you for the module itself. Always been a lifesaver.
Thanks for reading.

Best regards,

Laurent