Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DX] Split path.path.inc file into their respective modules #3182

Closed
opi opened this issue Jun 29, 2018 · 6 comments
Closed

[DX] Split path.path.inc file into their respective modules #3182

opi opened this issue Jun 29, 2018 · 6 comments

Comments

@opi
Copy link

opi commented Jun 29, 2018

https://github.com/backdrop/backdrop/blob/1.x/core/modules/path/path.path.inc

This file implements hook_path_info for 3 other core modules : node, taxonomy and user.

Quoting the path.path.inc top comment:

Path is an optional module in Backdrop, providing essentially a UI for the
core path functionality. Because this module may be disabled, it's up to this
module to provide implementations for other core modules.

I really don't understand the point here. I think we are facing the same issue as #2952 (comment) with hook_layout_context_info.

Some related issues : #2954 & #3024

--

PR backdrop/backdrop#2254

@jlfranklin
Copy link
Member

There is no reason node, taxonomy, and user can’t own their own hook implementations. They won’t be called unless path is enabled.

This may be a hold over from when Path was a contrib module. Core doesn’t implement hooks for non-core modules.

@opi
Copy link
Author

opi commented Jul 16, 2018

Core doesn’t implement hooks for non-core modules.

Indeed, It explains everything here. Thanks

@opi
Copy link
Author

opi commented Jul 27, 2018

backdrop/backdrop#2254 is ready for review

@klonos
Copy link
Member

klonos commented Jul 28, 2018

The PR looks good to me, but I would like someone with more knowledge on this to also take a look.

By removing the path.path.inc file, we have lost this docblock:

/**	
 * @file	
 * Path integration for core modules.	
 *	
 * Path is an optional module in Backdrop, providing essentially a UI for the	
 * core path functionality. Because this module may be disabled, it's up to this	
 * module to provide implementations for other core modules.	
 */

Not sure if I get things right, but does the above mean that every *_path_info() function should be checking if (module_exists('path'))? ...or am I missing the point here, and the core path functionality is all that's needed?

@opi
Copy link
Author

opi commented Jul 28, 2018

This docblock is actually wrong I guess. And implementing a hook for a disabled module is no harm

@quicksketch
Copy link
Member

Looks great. I've merged backdrop/backdrop#2254 into 1.x. This shouldn't be an API change (it's functionally identical), but the rearrangement might surprise some people so I've not backported it into 1.10.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants