[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"

Files: 
CommentFileSizeAuthor
#13 drupal.path_.inc_.patch28.46 KBcatch
Failed: 11245 passes, 202 fails, 53 exceptions
[ View ]
#8 drupal.path_.inc_.patch15.74 KBcatch
Failed: Failed to run tests.
[ View ]
#2 drupal.path_.inc_.patch15.75 KBsun
Failed: Failed to run tests.
[ View ]

Comments

Issue tags:+DrupalWTF

.

Status:Active» Needs review
StatusFileSize
new15.75 KB
Failed: Failed to run tests.
[ View ]

Indeed. :)

NOTE.

This patch

  • does nothing else than to move weird functions from path.inc into appropriate locations.
  • moves actual URL alias functions as-is into path.module
  • introduces hook_lookup_path() to munge $_GET['q'] in drupal_init_path()
  • changes site information settings to store the internal Drupal path instead of a (potential) path alias

That's... well... where's my WTF counter?

Status:Needs review» Needs work

The last submitted patch failed testing.

Priority:Normal» Critical

I can only guess that means: testbot depends on path.inc

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.

Major subscribe.

Status:Needs work» Needs review
StatusFileSize
new15.74 KB
Failed: Failed to run tests.
[ View ]

re-rolled. See what the bot says.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Let's work out why the test bot fails. But in the mean time this needs review.

+        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...

boombatower pointed out that PIFR probably depends on Clean URLs...

http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/project_iss...

StatusFileSize
new28.46 KB
Failed: 11245 passes, 202 fails, 53 exceptions
[ View ]

path.inc removed this time.

Status:Needs review» Needs work

The last submitted patch failed testing.

Issue tags:+API clean-up

Tagging for feature freeze.

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?

Version:7.x-dev» 8.x-dev
Priority:Critical» Normal

Moving this to Drupal 8.

Component:base system» path.module

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.

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.

Linking back to #320132: Make path.inc swappable in case people missed that one.

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().

http://buytaert.net/8-steps-for-drupal-8 let's stop saying smallcore.

i've started on this, very early work is here:

http://drupal.org/sandbox/justinrandell/1272704

Issue tags:+Platform Initiative

Title:path.inc your time has comeMove URL alias functionality into Path module
Issue tags:-DrupalWTF, -DIE

Let'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

Title:Move URL alias functionality into Path moduleMove URL alias functionality into a Path API module (still separate from the Path UI)

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 :)

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.

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 :)