| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | path.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | API clean-up, Framework Initiative |
Issue Summary
[tha_sun] If URL aliases can be optional, then there is no point in supporting them in the first place. Add hook_foo(), move that stuff into Path module.
[tha_sun] + if you want to, convert Path into Path + Path UI module.
[tha_sun] Or, better yet, Alias + Alias UI module.
[tha_sun] Am I mistaken, or will http://drupal.org/node/370454 eliminate that path alias bootstrap issue?
[Druplicon] http://drupal.org/node/370454 => Simplify page caching => Drupal, base system, normal, needs work, 19 IRC mentions
[catch] tha_sun: you mean for cached pages? I think so yeah.
[tha_sun] The question is: What prevents Path module from implementing hook_boot() ?
[catch] tha_sun: for drupal_init_path()?
[tha_sun] catch: not sure, basically yes...
[catch] tha_sun: probably not a lot.
[catch] tha_sun: also, we should de-op drupal_lookup_path()
[tha_sun] de-op?
[catch] drupal_lookup_path_alias() drupal_lookup_path_source()
[catch] drupal_static_reset()
[tha_sun] ah, de-$op
[catch] tha_sun: in that case, do we need path.inc at all?
[tha_sun] so, we should fix the cause.
[tha_sun] Interesting question
[tha_sun] :)
[catch] arg() is in there.
[catch] But could move to common.inc I think.
[tha_sun] huh?!
[tha_sun] catch: hook_boot() would run much earlier than DRUPAL_BOOTSTRAP_PATH is running now
[catch] tha_sun: arg() could move to common.inc
[tha_sun] sure thing
[catch] why is drupal_get_title in here?
[tha_sun] LOL
[tha_sun] Let's move check_markup() in there!
[catch] tha_sun: the only one that'd cause serious problems is drupal_is_front_page() if that gets used a lot.
[tha_sun] catch: That has nothing to do with URL aliases?
[catch] drupal_is_front_page?
[Druplicon] drupal_is_front_page: Check if the current page is the front page. => drupal_is_front_page() => http://api.drupal.org/api/function/drupal_is_front_page/6
[catch] tha_sun: apparently it does.
* catch never knew.
[catch] tha_sun: but looks like we should just store normal path for site front page same as the menu system does for links.
[tha_sun] catch: I'm looking at that monolothic path.inc right now. It just converts site_frontpage into a Drupal path. Guess what?
[tha_sun] heh
[tha_sun] system_site_settings_submit?
[tha_sun] bah
[tha_sun] system_site_information_settings_validate?
[Druplicon] system_site_information_settings_validate: Validate the submitted site-information form. => system_site_information_settings_validate($form, &$form_state) => http://api.drupal.org/api/function/system_site_information_settings_vali...
[tha_sun] We already validate.
[tha_sun] We just don't alter.
[catch] nice.
[tha_sun] Most probably -- a UX issue :P
[tha_sun] WTF? I'VE TOLD YOU! MY FRONTPAGE IS eat-my-dog-food NOT node/128412841234
[catch] tha_sun: if we do it for menu items, then they can go and die for the front page.
[catch] tha_sun: or we can remove all of path.inc down to that one function.
[tha_sun] But, who says that we can't invoke drupal_get_path_alias() for that #default_value ..? ;)
[catch] tha_sun: if module_exists('path') drupal_get_path_alias()
[tha_sun] (if Path module is enabled)
[catch] ugh, but yeah.
[catch] tha_sun: path.module could form alter!
[tha_sun] heheh :)
[tha_sun] yay!
[Druplicon] yeeeeeeeeeeeeeeeeeha!
[catch] tha_sun: mind if I paste this into a new issue.
[tha_sun] catch: JohnAlbin-style? "DIE path.inc DIE!!!" :-D
[catch] tha_sun: I'd already typed "path.inc your time has come"
Comments
#1
.
#2
Indeed. :)
#3
NOTE.
This patch
That's... well... where's my WTF counter?
#4
The last submitted patch failed testing.
#5
I can only guess that means: testbot depends on path.inc
#6
The patch applied fine on my local installation. The only thing I had to do was remove every entry of path.inc out of my registry table. Since patches seem to be applied before the bot installs Drupal I don't really see what could go wrong.
#7
Major subscribe.
#8
re-rolled. See what the bot says.
#9
The last submitted patch failed testing.
#10
Let's work out why the test bot fails. But in the mean time this needs review.
#11
+ if ($src = db_query("SELECT src FROM {url_alias} WHERE dst = :dst AND language IN(:language, '') ORDER BY language DESC", array(+ ':dst' => $path,
+ ':language' => $path_language)
+ )->fetchField()) {
I assume that part is just moved from one place to another, but it's imho usual to move multi-line db statements out of the if into a temporary $var. Atleast that's what I'm doing :)
When looking at the patch, I see only added (moved) functions, and nothing get's removed, shouldn't the patch remove path.inc? Maybe that's a problem, if the testbot manually includes path.inc, it would die with an Fatal Errror. You could try with an empty (and not removed) path.inc file..
I do have a similiar problem over at #145164: DX: Use hook_variable_info to declare variables and defaults and I can simply not reproduce why the tests don't run.. that one over there *did* run in an earlier version...
#12
boombatower pointed out that PIFR probably depends on Clean URLs...
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/project_iss...
#13
path.inc removed this time.
#14
The last submitted patch failed testing.
#15
Tagging for feature freeze.
#16
So essentially this makes path aliasing optional? Sounds great. I looked at the patch a bit, seems basically to be moving the path-related functions into more logical places.
Will the calls to drupal_function_exists() have to be replaced by module_exists() though because of the registry changes?
#17
Moving this to Drupal 8.
#18
I'd like to move this to path.module so I can keep track of all these path changes. Ideally it would be under the 'path system' component.
#19
IMO one of the first responsibilities of #smallcore in terms of actual code changes should be clarifying the currently entangled responsibilities between /includes and /modules (both inter and intra) - which means this and #187398: Re-split locale module for starters.
#20
Linking back to #320132: Make path.inc swappable in case people missed that one.
#21
Splitting the functions in path.inc to path.module and the current path.module to path_ui.module makes a whole lot of sense. The new path.module (API only) can implement hook_url_inbound_alter() and we can get rid of drupal_get_path_alias and the call to drupal_lookup_path() in drupal_get_normal_path().
#22
http://buytaert.net/8-steps-for-drupal-8 let's stop saying smallcore.
#23
i've started on this, very early work is here:
http://drupal.org/sandbox/justinrandell/1272704
#24
#25
#26
Let's get back to this after #1183208: Change notification for: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support landed.
Also, let's make sure to write a proper issue summary here (an IRC log is not really useful ;)).
Lastly, related issue: #1269742: Make path lookup code into an pluggable class
#27
Slight retitling based on the above discussion.
I seem to recall another discussion about this long ago where it was pointed out this would be a particularly bad place to introduce a regression in the (existing) UI vs API separation. Because it would be bad if turning off the Path module (to get rid of the UI or replace it with a different UI) also had the effect of breaking every link on the internet which pointed to your site :)
#28
Path aliasing and an API for it should be a core subsystem, in /lib. It's going to be a dependency for the kernel listeners to function properly. It should contain a complete API, but *no* UI. At all. The UI should be entirely in the module, which should consist of ONLY forms and submit hooks that call the API. No actual functionality or additional APIs whatsoever.
#29
That's pretty much what we have already (except for lib vs includes).
Are you suggesting this issue should be "won't fix", or...? If the proposed new module only existed to move the
drupal_path_initialize()out of the bootstrap (but everything else still remained a core subsystem) I'm not sure it would really be worth it. So maybe this really does need an issue summary after all :)