[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"
Comment | File | Size | Author |
---|---|---|---|
#13 | drupal.path_.inc_.patch | 28.46 KB | catch |
#8 | drupal.path_.inc_.patch | 15.74 KB | catch |
#2 | drupal.path_.inc_.patch | 15.75 KB | sun |
Comments
Comment #1
sun.
Comment #2
sunIndeed. :)
Comment #3
sunNOTE.
This patch
That's... well... where's my WTF counter?
Comment #5
sunI can only guess that means: testbot depends on path.inc
Comment #6
j.somers CreditAttribution: j.somers commentedThe 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.
Comment #7
Dave ReidMajor subscribe.
Comment #8
catchre-rolled. See what the bot says.
Comment #10
catchLet's work out why the test bot fails. But in the mean time this needs review.
Comment #11
BerdirI 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...
Comment #12
sunboombatower pointed out that PIFR probably depends on Clean URLs...
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/project_iss...
Comment #13
catchpath.inc removed this time.
Comment #15
sunTagging for feature freeze.
Comment #16
EvanDonovan CreditAttribution: EvanDonovan commentedSo 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?
Comment #17
catchMoving this to Drupal 8.
Comment #18
Dave ReidI'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.
Comment #19
catchIMO 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.
Comment #20
RobLoachLinking back to #320132: Make path.inc swappable in case people missed that one.
Comment #21
Dave ReidSplitting 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().
Comment #22
chx CreditAttribution: chx commentedhttp://buytaert.net/8-steps-for-drupal-8 let's stop saying smallcore.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedi've started on this, very early work is here:
http://drupal.org/sandbox/justinrandell/1272704
Comment #24
RobLoachComment #25
RobLoachComment #26
sunLet's get back to this after #1183208: 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 a pluggable class
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedSlight 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 :)
Comment #28
Crell CreditAttribution: Crell commentedPath 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.
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedThat'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 :)Comment #42
fjgarlin CreditAttribution: fjgarlin as a volunteer and at Drupal Association commentedComment #43
smustgrave CreditAttribution: smustgrave at Mobomo commentedCurrently going through the end of the queue to see what could still be relevant for D10.
Wonder if this is still needed for D10?
Comment #44
catchWe did this in #3084983: Move all the code related to path aliases to a new (required) "path_alias" module