Problem/Motivation

#3140535: Automated Drupal Rector fixes made a lot of progress getting this module D9-ready. However, the bot couldn't solve everything, and tests are currently failing when you try to run this on a 9.2.x core test site.

Steps to reproduce

  1. Install Drupal core 9.2.x.
  2. Install the latest 8.x-1.x of fieldable_path. Apply latest patch from #3140535: Automated Drupal Rector fixes
  3. ./vendor/bin/phpunit -c core/phpunit.xml modules/contrib/fieldable_path/tests/src/

Proposed resolution

Get the tests passing in D9 by finishing the port.

Remaining tasks

  1. "The deprecated hook hook_path_insert() is implemented in these functions: fieldable_path_path_insert(). It will be removed before Drupal 9.0.0. Use hook_ENTITY_TYPE_insert() for the 'path_alias' entity type instead. See https://www.drupal.org/node/3013865."
  2. "The deprecated hook hook_path_update() is implemented in these functions: fieldable_path_path_update(). It will be removed before Drupal 9.0.0. Use hook_ENTITY_TYPE_update() for the 'path_alias' entity type instead. See https://www.drupal.org/node/3013865."
  3. "The deprecated hook hook_path_delete() is implemented in these functions: fieldable_path_path_delete(). It will be removed before Drupal 9.0.0. Use hook_ENTITY_TYPE_delete() for the 'path_alias' entity type instead. See https://www.drupal.org/node/3013865."
  4. Make sure the existing tests pass on D9.
  5. Maybe add some additional test coverage to have a bit more confidence in the results.
  6. Reviews / refinements.
  7. Commit.
  8. Tag a new stable release (8.x-1.0-rc6)
  9. Rejoice

User interface changes

Hopefully not.

API changes

  • Deprecate the legacy FieldablePathController::checkEntityAndSyncPath() method.
  • Add a new FieldablePathController::syncEntityPath() method that takes a PathAliasInterface parameter.

Data model changes

TBD, probably not.

CommentFileSizeAuthor
#9 3217038.6_9.interdiff.txt651 bytesdww
#9 3217038-9.patch5.12 KBdww
#6 3217038.5_6.interdiff.txt500 bytesdww
#6 3217038-6.patch5.07 KBdww
#5 3217038.3_5.interdiff.txt2.68 KBdww
#5 3217038-5.real_.patch4.74 KBdww
#5 3217038-5.legacy-hooks.patch3.29 KBdww
#3 3217038-3.patch4.27 KBdww

Comments

dww created an issue. See original summary.

dww’s picture

dww
we/he/they
commented

Filled in the D9 porting info on the project page to point here.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3217038-3.patch4.27 KB

Eeek. https://www.drupal.org/node/3013865 changed a lot of the core API around path aliases.

I don't know if anyone's actually using/extending our FieldablePathController class, but I didn't want to go radically changing the methods in there to match how core now deals with aliases.

So here's a work-in-progress patch that deprecates the existing FieldablePathController::checkEntityAndSyncPath() method with the old interface, and adds a new syncEntityPath() method that takes a PathAliasInterface argument, not a string and an array.

The tests are getting further now, on both D8 and D9. But they're failing when we're trying to delete aliases. Still digging on that front, but I wanted to post what I've got so far, especially to get feedback from @Spleshka on this approach.

Status: Needs review » Needs work

The last submitted patch, 3: 3217038-3.patch, failed testing. View results

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new 3217038-5.legacy-hooks.patch3.29 KB
new 3217038-5.real_.patch4.74 KB
new 3217038.3_5.interdiff.txt2.68 KB

Once again, yay for tests! ;) I completely borked the API reshuffle for the delete case, which is exactly what the tests were telling me.

Here's a much better pair of patches. ;) Both of them work on D8. Only one of them works on D9:

  • The 3217038-5.legacy-hooks.patch version only changes the FieldablePathController class, but leaves the legacy hook implementations. So this still works on D8 due to core's BC layer to invoke the deprecated hooks. It fails on D9 since without implementing the new hooks, nothing's getting triggered and the tests fail.
  • The 3217038-5.real.patch should now pass on both branches. It's the identical changes to FieldablePathController, but also changes to fieldable_path.module to implement the correct core hooks.
dww’s picture

dww
we/he/they
commented
StatusFileSize
new 3217038-6.patch5.07 KB
new 3217038.5_6.interdiff.txt500 bytes

Here's a slightly better version of the full patch. I was using \Drupal::entityTypeManager() in a spot, but we're now extending ControllerBase which has its own entityTypeManager() method we should use, instead.

dww’s picture

dww
we/he/they
commented
Assigned: dww » spleshka
Issue summary: View changes

I had to requeue the 9.1.x tests for #5 and #6 since I forgot core bumped the minimum mysql version. So the test was failing trying to install Drupal with mysql 5.5.

All the test results are now as expected:

I believe this is now ready, but I'm going to leave this open and assign to @Spleshka to get the other maintainer to confirm they like this approach before I commit it.

Stay tuned...

spleshka’s picture

spleshka
he/him
Primary language English
Location UAE
commented
Assigned: spleshka » Unassigned

I looked through the code of the patch from #6 and the changes look completely okay to me. Unfortunately I don't have a real project to test the upgraded version of the module in the real life, but if others are happy with how it works then I'm all up for getting it committed. Shall I mark it as RTBC at this stage or let's wait for others to test it first?

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 3217038-9.patch5.12 KB
new 3217038.6_9.interdiff.txt651 bytes

Based on a Slack chat, here's an update to the version strings in the deprecation message to be more accurate and hopefully less confusing.

Also, I added a CR about the API change, so the @see comment immediately after @deprecated can follow the conventions and point to the CR about the change.

I think if you and I are both happy, we should just commit it. ;) But I'll leave this open for the next ~12 hours while I sleep and deal with the start of my work day tomorrow, in case anyone else appears who can review / test this.

Thanks again,
-Derek

spleshka’s picture

spleshka
he/him
Primary language English
Location UAE
commented
Status: Needs review » Reviewed & tested by the community

All good, let's get it in! :)

  • dww committed 24d4c61 on 8.x-1.x 
    Issue #3217038 by dww, Spleshka: Port fieldable_path to Drupal 9
dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Thanks! Committed and pushed to 8.x-1.x branch.

Also tagged and published the 8.x-1.0-rc6 release.

Status: Fixed » Closed (fixed)

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