Add pathauto to core

[maxpearl]

I know there have been a lot of arguments about this for D8, but in terms of the goals of backdrop, and ease of use, it seems that perhaps there is a good argument for this.

I'd love to see it happen.

(And I don't know what the status of token is/will be...?)

[quicksketch]

Thanks for opening this @michellemurrain, I think Pathauto would be an absolutely great fit for Backdrop. There are a couple of issues that would be required to make this possible though:

• Field-level tokens need to be provided by Field module (instead of token.module).

• We'd need a token browser built-into system.module (or wherever appropriate).

  But overall those aren't insurmountable problems. Token module and pathauto modules shouldn't be needed, IMO. They should be part of system and path modules respectively.

[mikemccaffrey]

+1 for moving Token and Pathauto into core. Refactoring them into the system and path modules would be great, but keeping them as standalone modules for the time being doesn't seem like the end of the world.

[maxpearl]

@quicksketch, would that suffice as a 1.0 milestone effort, with the future being to refactor them into field and system.module? (I probably can accomplish that, in fact, I was getting close before I read your comment - the refactoring... dunno, might be above my pay grade.)

[quicksketch]

I should point out that the token system is built-into D7 already. It's only the UI and the additional tokens for Field that are in the token.module itself. That's why I'm saying it should be merged into other modules, since token.module itself is basically just stubbing in functionality for other modules at this point already.

[maxpearl]

Actually, there seems to be a lot more in the token module than just the browser and the additional field tokens. It seems that varied things ended up getting left out of core, like correcting entity type names, token cache, etc.

Anyway, I'm out of my depth. I could almost certainly get the token module to work in backdrop core, as well as put pathauto into the path module (that seems rather logical, and doable.) Can we do that, then tackle refactoring later? Or I'm happy to leave this to someone else entirely.

[DirectorHaas]

@quicksketch Did some fast checks on the current state of the D7 token module, starting to incorporate into the Pathauto module. If D7 token runs in core (just change the .info files) in it's own dir, it almost works about 90%, pulling it into Pathauto will be a little more work. Since token has the interface, does it make any sense to run token in it's own /core/ token directory? May just need a little clarification about #87 (comment)
Thanks!

[klonos]

Where do we stand on this one? Do we see it as a 1.x or 2.x target?

[quicksketch]

It's not a 1.0 target but we could add it in a subsequent 1.1 or 1.2 update.

[klonos]

Ok, thought so. I just wanted an "official" reply on the matter. Thanx ;)

[quicksketch]

Let's make this a target for 1.1.0. The initial release of Backdrop is due out next month, let's shoot for Pathauto as one of the first minor releases.

[klonos]

I would like to suggest to also merge in the features provided by Sub-pathauto (Sub-path URL Aliases) / Extended Path Aliases as well.

[ghost]

I'd also like to suggest the addition of Redirect-type functionality as well (it's a recommended addition to Pathauto on D.o).

[enzolutions]

I did a version without drupal compatibility https://github.com/backdrop-contrib/pathauto @BWPanda you have documentation about that integration.

@quicksketch are you agree with integration with redirect, Sub Pathauto and Extended Path Aliases?

[quicksketch]

@enzolutions Sure, I think bundling them together would be fine. Long-term, I'd love to see all this functionality built-into path module directly, but we may need to prune some of the more difficult/edge-case code from the projects (e.g. bulk create and regenerate code) and move it into a separate contrib project.

[klonos]

Maybe we should keep the it simple in core and provide an "Advanced" section where we "hide" any "scary" settings from the main UI. I like how WP offers very basic settings through a simple UI:

[quicksketch]

@jenlampton made a separate issue for implementing token into core at #480.

Looking at the parts of Pathauto, I think it includes these main components:

• Path pattern administration.
• Hooks for various data types to create aliases (nodes, users, terms).
• Bulk creation/updates.
• Bulk deletion.

The first two are obvious requirements for core and is what people generally think of as the functionality Pathauto provides. What about the last two for bulk create, update, and delete? Should that be included in core? I might be painting this based on my past negative experiences with bulk updates, but if that functionality is not 100% stable it may be better left to a separate contrib module.

[klonos]

...but if that functionality is not 100% stable it may be better left to a separate contrib module.

Perhaps that's exactly why it should be in core. Saying that because in Drupaland issues were resolved faster if they were against core (guess they effected more people).

[quicksketch]

Touché. 😉

[ghost]

I like and use the bulk update/delete functionality. One thing I'd suggest if we do add it to core (which I think we should) is that the Bulk Update functionality should include updating existing aliases (or at least have an option for this). It's annoying to have to bulk delete all aliases then bulk create them just because I updated something...

[ghost]

Also, we should add 'update' and 'delete' as VBO's to the alias page (admin/config/search/path) rather than as separate tabs (make consistent with the rest of Backdrop).

[klonos]

AFAICT, pathauto has the option to bulk update for some time now. There's no need to have any special option for it. If you want to update already existing aliases, all you need to do is select all relevant content from the Content admin page (/admin/content) and select the "Update URL alias" option available in the "Update options" drop-down menu.

Anything I'm missing here?

[klonos]

...or maybe you meant something like a "Save configuration and update existing aliases" button/option in the Patterns config page (/admin/config/search/path/patterns). Oooh, I'd like that too! Yes please!!

[klonos]

...although, I don't know how this feature would work together with the option to not auto-generate aliases (https://www.drupal.org/project/pathauto_persist / Merge in pathauto_persist module functionality to prevent losing manual aliases with node_save() calls). Another checkbox/option to respect the "Generate automatic URL alias" for each piece of content?

[ghost]

Anything I'm missing here?

That only works for Content (nodes), but other entities can also have aliases that need updating (Taxonomy Terms, Users, etc.)

[jenlampton]

@klonos the bulk update action you are referring to is the one that @quicksketch mentioned as being "not 100% stable", and I think that's a polite way of putting it. I've personally had very little success getting those bulk options to work - though I do wish for actually-working bulk actions at some point on pretty much every single site. Nobody wants to manually update paths for 10,000 blog posts!

I'd hate to see such a useful and in-high-demand feature be excluded. I'm not sure what the problem is with the current state of pathauto's bulk update operations, but I think we should aim to include that feature. It's important.

[klonos]

And lastly to help the consistency of Pathauto, I built-in Pathauto Persist directly as part of the core path system...

🎉

[docwilmot]

Are these new hooks replacing the originals or in addition to the originals? I'm thinking change records.

[quicksketch]

Are these new hooks replacing the originals or in addition to the originals? I'm thinking change records.

The changes to use hook_entity_[action]() are all within path.module and shouldn't affect the API of any other module. The changes up above in #87 (comment) are replacements for the old hook_pathauto_* hooks.

[serundeputy]

installed beautifully on a fresh backdrop 1.x;
update.php ran great!

aliases for
* Article
* Basic Pages
* Users
* Taxonomy Terms

I can manually set aliases (uncheck the box), and reset to auto (check the box) all working perfectly.

working as expected. Respecting patterns and tokens from /admin/config/search/path/patterns.
Respecting character separator.

Spun up a few hundred Articles, Basic Pages, Taxonomy Terms and Users w/ devel all taking aliases as expected!

A few php warnings on form saves:

[quicksketch]

Great, thanks @serundeputy! It looks like that particular issue is a problem with the token integration, not this set of Pathauto changes. I filed a new issue at #906.

[serundeputy]

with pathauto contrib and then git co 87/pathauto:

[serundeputy]

I think this could be a settings.php and files/conf_ more than pathauto, token upgrade problem.

I'll try to detangle tomorrow.

[serundeputy]

@quicksketch seems i'm still getting the error in the screenshot: can not redeclare token_type_load().

here is what i did.

• installed backdrop-1.0.5
• installed token
• installed pathauto
• made a page ; checked to see that it had automatic alias

pulled down a copy of backdrop 1.x in a different directory;

• moved the core directory out of my backdrop-1.0.5 installation
• replaced with the core directory of the 1.x branch;
• visit site in browser.

~Geoff

[quicksketch]

Hi @serundeputy, I made a few fixes in the 1.x branch that might help prevent WSOD on the update.php page, including #910 and #722. I tried this all from scratch again today:

• Fresh install of 1.0.5. Install Pathauto and Token latest releases.
• Configure Pathauto to add aliases for different types of content. Adjust any settings as well.
• Git checkout 87/pathauto to replace 1.0.5.
• Visit the homepage of the site. At this point I get this warning:
  
  Some other pages will also cause errors, such as attempting to save a node or visiting the path pattern settings pages. These are usually fatal duplicate function errors.
• However, at this point I can also visit update.php (with no errors if I do this first). And running update.php works successfully with no errors. Visiting path pattern settings I can see that all aliases are maintained and work correctly.

[quicksketch]

Hi @serundeputy, I was writing the above post when you posted as well. I'm not sure what to do here. It doesn't seem possible that we could totally prevent duplicate function errors between token.module and token.inc unless we rename the functions (thus causing more compatibility breaks). Historically, errors on your site prior to running update.php have been considered "normal", so if we can at least get users to update.php without errors than that's the minimum level of acceptability.

Of course, we can (and probably will) also recommend that any users have Token or Pathauto modules remove those modules from their installation prior to doing the update to 1.1.0. But it would clearly be preferable to be able to disable those module without any other manual changes, so the upgrade process matches our documentation.

[quicksketch]

Ah! I got it. We can add the list of merged modules into module_list(), specifically removing ones that have been merged into core. That will make it so that Backdrop does not load the .module file for these modules. I'll update the PR and then we can give this another go.

[quicksketch]

@serundeputy This latest commit should prevent the old Token or Pathauto modules from be executed, even if they are still enabled and present:
quicksketch/backdrop@42811db

In my testing, this prevents all critical errors on the site before update.php is executed. It should correct the problems with update.php itself as well, as the duplicate functions should all be avoided.

[klonos]

I manually patched the files from commit quicksketch/backdrop@42811db because I was getting the same Cannot redeclare token_type_load() fatal error as @serundeputy

The site loads fine, but trying to visit the site status page (/admin/reports/status) throws another fatal:

Fatal error: Cannot redeclare token_get_token_problems() (previously declared in C:\www\backdrop\core\includes\token.inc:404) in C:\www\backdrop\modules\token\token.install on line 332
Call Stack
#   Time    Memory   Function                                                                           Location
1   0.0002  244200   {main}( )                                                                          ...\index.php:0
2   0.7478  13877040 menu_execute_active_handler( )                                                     ...\index.php:21
3   0.7479  13878192 layout_route_handler( )                                                            ...\menu.inc:520
4   0.8059  14453304 LayoutRendererStandard->render( )                                                  ...\layout.module:445
5   0.8059  14455856 LayoutRendererStandard->renderLayout( )                                            ...\layout_renderer_standard.inc:312
6   0.8061  14460968 LayoutRendererStandard->renderBlocks( )                                            ...\layout_renderer_standard.inc:331
7   0.8061  14461920 LayoutRendererStandard->renderBlock( )                                             ...\layout_renderer_standard.inc:408
8   0.8061  14462000 BlockLegacy->getContent( )                                                         ...\layout_renderer_standard.inc:450
9   0.8061  14462264 BlockLegacy->buildBlock( )                                                         ...\block_legacy.class.inc:57
10  0.8061  14463152 module_invoke( )                                                                   ...\block_legacy.class.inc:30
11  0.8061  14464248 call_user_func_array:{C:\www\backdrop\core\includes\module.inc:859} ( )            ...\module.inc:859
12  0.8061  14464832 system_block_view( )                                                               ...\module.inc:859
13  0.8103  14941344 call_user_func_array:{C:\www\backdrop\core\modules\system\system.module:2231} ( )  ...\system.module:2231
14  0.8104  14941664 system_status( )                                                                   ...\system.module:2231
15  0.8143  15152112 backdrop_load_updates( )                                                           ...\system.admin.inc:2166
16  0.8301  16518608 module_load_install( )                                                             ...\install.inc:84
17  0.8301  16518656 module_load_include( )                                                             ...\module.inc:296

[klonos]

...also tried downloading the whole tree (87/pathauto), but same thing and another fatal when trying the "Run updates" page (/core/update.php?op=info):

Fatal error: Cannot redeclare token_get_token_problems() (previously declared in C:\www\backdrop\core\includes\token.inc:404) in C:\www\backdrop\modules\token\token.install on line 332
Call Stack
#  Time    Memory    Function                  Location
1  0.0011  329400    {main}( )                 ...\update.php:0
2  0.4213  13924040  backdrop_load_updates( )  ...\update.php:484
3  0.4404  15242576  module_load_install( )    ...\install.inc:84
4  0.4404  15242624  module_load_include( )    ...\module.inc:296

[quicksketch]

Thanks @klonos! I hadn't thought that we'd need to do the same thing to prevent .install files from being loaded, but now that you mention the problems it seems obvious that this would happen. I'll work on finding a way to prevent token.install from loading as well.

[quicksketch]

I've updated the PR to include a fix to prevent .install files for "merged" modules from being loaded in update.php or the status report page. I amended the last commit, so if you're applying things manually the only thing that changed was the addition to backdrop_load_updates(): quicksketch/backdrop@118e900#diff-eb90f57f441b14b60d6f4670ba018affL80

[serundeputy]

this works beautifully ; thanks so much @quicksketch

• upgraded from 1.0.5 to 87/pathauto w/ no contrib modules
• upgraded from 1.0.5 to 87/pathauto w/ token installed
• upgraded from 1.0.5 to 87/pathauto w/ token and pathauto installed
  • preserved existing paths

😄 🙇 😄

[quicksketch]

Woot woot! I'll probably merge this in later this evening. Everything else we can get in before Friday will just be icing on the 🍰.

[klonos]

Manually patched diff and now the "Status report" and "Run updates" pages are accessible once again. Thanx!

PS: While running the update.php, the section of the pending updates reads:

path module

• 1000 - Upgrades Drupal 7 Pathauto variables to Path config.
• 1001 - Moves Backdrop Pathauto configuration to Path module configuration.

Is the "Drupal 7" intentional there? Asking because my test site was not a D7 upgrade - simply a Backdrop 1.0.0 that I upgrade over time as new releases/patches come out.

PPS: #913

[quicksketch]

Is the "Drupal 7" intentional there? Asking because my test site was not a D7 upgrade - simply a Backdrop 1.0.0 that I upgrade over time as new releases/patches come out.

Yes, "Drupal 7" is intentional. We provide an upgrade path directly from Drupal 7 AND Backdrop 1.0.0. So if regardless of whether you used Backdrop's Pathauto or D7's Pathauto, both are migrated.

[quicksketch]

From the looks of the bug reports @klonos and others have been filing (here and in other issues), most of the problems introduced are due to Token's merging (because of either function overlaps or misported tokens). Pathauto itself so far seems to be working well. To get us "feature complete" and able to test everything together, I've merged this into 1.x. woot woot!

[klonos]

Thanx, that's great!