Move request path handling into context

Makes sense. Probably something like ['native-path'] and ['aliased-path'], one for the "raw" path and the other for the after-alias-dereferencing.

Those are probably the wrong names, but yes we should have something like that. :-)

Originally wanted to note that we call these current_path() and request_path() "elsewhere", but...

path:system and path:alias (present tense) sound pretty good.

Agreed! Maybe even default "path" to "path:system".

path:system (always node/5) and path:alias (about/history if available, or node/5 if not) sounds fine to me. The only caveat would be if we want "path" to be "the literal value out of the request, whichever it is", or if we tell people to just hit http:get:q directly for that.

I'm a bit wary about the direction of this.

#464164: Move URL alias functionality into a Path API module (still separate from the Path UI) would move path aliases to an implementation of hook_url_inbound_alter() (probably), so we'd no longer have a specific bootstrap path alias lookup step.

Also http:get:q is not even taken from $_GET['q'] in the first place all the time anyway, sometimes it comes from $_SERVER['request_uri'], see http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/reques...

So for any request we will always have path:request (which comes from $_GET['q'] or $_SERVER['request_uri'] with some munging). I don't think we should hard code the alias concept in here since even if the path.inc switch around doesn't happen, there are still valid request uris that are neither system paths nor aliases that can be mapped to a system path via that hook.

path:system makes sense for system paths (we should change the 'source' column in path.inc to match this terminology).

Then there's the further possibility that you request a system path like node/1, but this is also aliased, say to 'about'. In those cases we don't do the reverse lookup for path aliases matching that path in core (this is left to something like globalredirect). That seems like the main reason to have a third path:alias alias but feels very optional to me.

So for me that means path:request (can't currently be got without munging so worth an alias), and path:system (either the internal system path its mapped to via aliases or url_alter, or whatever came in if nothing matches which would end up eventually as a 404). Then there's a blank area in my mind for path:alias which is the reverse lookup of system path to alias (or reverse engineering hook_url_inbound_alter() which might be impossible and would depend on the implementation).

sub too

OK, we talked through this in IRC today, and came up with the following.

Right now, Drupal has 4 path-related utility functions and one raw global. From chx:

Say you have

http://example.com/drupal/documentation?page=1

Let's presume documentation is an alias for foo/26419

Then after a full bootstrap

$GLOBALS['base_url'] is http://example.com/drupal
base_path() (aka $GLOBALS['base_path']) is /drupal/ 
request_uri() is /drupal/documentation?page=1
request_path() is documentation
current_path() is foo/26419

And as of Drupal 7, $_GET['q'] is not actually $_GET['q'], but over-written by request_path(), the one time it's used. A rare occurance of Drupal implementing backward compatibility. :-) See chx's blog post for more details.

In HttpFoundation, these correspond to (I believe):

base_url: getScheme() . '://' . getHost() . getBaseUrl()
base_path: getBasePath()
request_uri: getRequestUri()
request_path: No direct equivalent, as the code in request_path() is kinda specific
current_path: No equivalent due to path aliasing

So, what we should therefore do is:

- Add a http:base_url key to the Http handler that does the above.

- Add a http:base_path key to the Http handler that returns $request->getBasePath().

- Add a http:request_uri key to the Http handler that returns $request->getRequestUri()

- Rename the http:url key in the Http handler to http:uri, for consistency with HttpFoundation.

- Add a path:raw context key that is more or less the current logic of request_path() (tweaked to reference other handlers as needed). Thisis always available. This replaces request_path().

- Add a path:system key that gives the path:raw, de-aliased down to the system path if applicable. If the path-dealiasing logic is not yet available because we're too early in the bootstrap, throw an exception so that the developer knows it immediately. (Hopefully later refactoring will allow dealiasing earlier in bootstrap, but TBD.) This replaces current_path().

(Some massaging for leading/trailing slashes may be necessary along the way.)

Once those are done, we can and should eliminate the base_url global and the other four functions listed above. All should be accessible via context.

Unless catch has an objection to the above, this is our next major hitlist item and I will update the summary accordingly. Catch, please give us a +1/-1 here.

Latest plan here looks fine to me. Not 100% on the naming but nothing jumps out either.

Note that there are places such as language negotiation that can affect current path that run during bootstrap that will need the option to run here as well ( and inbound alter etc), so there is more of a gap between request path and system path than de-aliasing.

The proposal here makes a whole lot of sense and is +1 from me. The only thing that seems odd is path:system and path:raw. I can think of raw being both the URL a user types in and also the Drupal 'system' path that is un-aliased as 'raw'. What if we used path:internal and path:external. I think those are more self-describing.

I made a feature branch: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/1262...
I'm not 100% sure it is correct but it is a start based on this discussion.

I'm new with git commandline, sometimes I do something wrong or I forget to track branches. Srry if that happens , still learning.

Srry for the ugly branch commits...

1) I think I managed to complete the http handler.

Got the following results :).

http:uri ==> http://localhost/drupal8/admin/modules?render=overlay
http:base_url ==> http://localhost/drupal8
http:base_path ==> /drupal8/
http:request_uri ==> /drupal8/admin/modules?render=overlay

I wondered what would happen if request->getBasePath() was empty. After googling I found this in the symphony docs
"http://localhost/index.php returns an empty string"

So in our case with this line:

$this->params[$property] = $this->request->getBasePath() . '/';

http://localhost/index.php will return "/"

This is what we need. I tried to remove the global but it is connect with base_url and some more globals (first time I look at this and I think it's a mess :) )

2) I fail in making the path handler.

3) request_uri() function

Can we replace the entire function in bootstrap.inc with

  $context = drupal_get_context();
  return $context->getValue('http:request_uri');

??? :)

4) $GLOBALS['base_url']

- Why isn't there a wrapper function like base_path()?
==> it would make it easier to convert to $context, else we have duplicate the same code a lot

- hmm this is used in tests, maybe the handlers aren't loaded yet in some tests...

Made a typo, now the path handler "does something" for path:raw

But it need to be refactored. See #9: "tweaked to reference other handlers as needed"

And sadly it doesn't return the alias... (as I think it is supposed to be)
But calling request_path() returns the alias... o_O

New results:
------------
http:uri ==> http://localhost/drupal8/alias
http:base_url ==> http://localhost/drupal8
http:base_path ==> /drupal8/
http:request_uri ==> /drupal8/alias
path:raw ==> node/1
http:query:q ==> node/1
get_q ==> node/1
request_path ==> alias

Cleaner branch: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/1262...
I need some help on implementing the path handler.

Ok path handler kinda works :)
It's time to give my branch a review.

I implemented path:raw and path:system by copy pasting existing functions. Those function were hardcoded steps in the bootstrap function. Now they get their value when they are loaded for the first time.

Crell had the idea to use the http context in stead of those uglt $_GET['q'] and SERVER calls. But this means we also have to pass $context to the getValue of path:system or path:raw. Is that what we want?

I couldn't throw away $_GET['q'] = request_path(); yet...
For some reason $_GET['q'] doesn't exist before doing this.
Can someone see why this isn't possible yet and how we can fix it?

The last commit is a test commit without that one your site will totally brake :)
I'm not sure we can split this branch into smaller pieces. It's possible we have to check every instance of $_GET['q'] in code before this will work again properly and pass tests...

Ok path handler is working now.

Steps that need to happen before everything works

1) Commit the improved http handler and the new path handler as a seperate commit

NOTE: path:raw will not work yet in this phase because $_GET['q'] gets rewritten (in drupal_path_initialize) at the end of the bootstrap proces. We don't know when path:raw will be called...

2) Remove drupal_path initialize and Replace every "call" of $_GET['q'] with path:raw or path:system.

- The first step fixes path:raw and path:system
- The second step makes sure drupal still works

3) Remove path_request()

- You can acces the request by using context (path:raw) so it's not a regression :)
We can move path_request outside bootstrap and link it to path:raw so developpers don't need to upgrade their code

Ok Crell told me I had to make 2 seperate handlers.

New branch: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/1262...

I made 4 commits
1) Added path handlers and added some extra stuff to the http handler
2) Rewrote $_GET['q'] to ->getValue('path:system') were possible. There are some cases left I wasn't sure off, you can find them easiky by grepping $_GET['q']

QUESTION: I can't find any reason why we aren't using current_path() instead as current_path() now returns ->getValue('system:path'). Is there a reason for that?

3) Removed drupal_initialize_path()
4) Rewrote request_path() // calls 'path:raw'

Crell asked me something about unit tests, but I'm not sure what can be tested here. If functions like current_path() don't fail in the unit tests we are safe. Or not?

No, the current tests are all integration tests. We want/need unit tests just of the new handler to ensure that the logic inside it is accurate, in isolation from anything else in Drupal.

Added a unit test, and updated #19.

Adding a proper patch so that this is reviewable...

+++ b/includes/Drupal/Context/Handler/HandlerPathRaw.php
@@ -0,0 +1,51 @@
+use \Drupal\Context\Handler;

This line is unnecessary, since we're already in that namespace.

+++ b/includes/Drupal/Context/Handler/HandlerPathSystem.php
@@ -0,0 +1,27 @@
+use \Drupal\Context\Handler;

This line is unnecessary, since we're already in that namespace.

+++ b/includes/bootstrap.inc
@@ -2521,9 +2514,11 @@ function drupal_maintenance_theme() {
+  $context = drupal_get_context();
+  $q = $context->getValue('path:system');

If we're only using the $context object once, you can chain getValue() off of the end of drupal_get_context().

Also, here and elsewhere, $q is a poor variable name. It only makes sense as a historical reference to when we hacked $_GET['q'] (as in, what we do now), but anyone reading this later won't understand what $q is. Instead, it should have some common meaningful name that matches the context key. I recommend $system_path.

The same is true throughout the patch, but I won't call it out each time so that this review isn't too huge. :-)

+++ b/includes/bootstrap.inc
@@ -2735,45 +2730,8 @@ function language_default() {
 function request_path() {

Is there a need to keep this function at all as a dumb wrapper? If it's just for temporary BC (which is OK), add a @deprecated tag to its docblock so we know we should kill it eventually.

+++ b/modules/simpletest/tests/context.test
@@ -546,3 +546,32 @@ class ContextHTTPTestCase extends ContextTestCases {
+  function testPathRawProperty() {
+    $butler = new \Drupal\Context\Context();
+
+    $params = array('values' => array('parameters' => array('q' => 'alias')));
+
+    $butler->setHandler('http', '\Drupal\Context\Handler\HandlerHttp', $params);
+    $butler->setHandler('path:raw', '\Drupal\Context\Handler\HandlerPathRaw', $params);
+    $butler->lock();
+
+    $this->assertEqual($butler->getValue('path:raw'), $params['values']['parameters']['q'], t('Raw path is fetched successfully.'));
+  }

path:raw doesn't need $params, as it's not being configured. It certainly doesn't need the same params that the http handler does.

What this is testing, in practice, is just the "there is a $_GE['q']" path within the handler. It won't test what happens for the other possible paths through the class (request_uri, php_self, etc.)

Thinking about it, actually, we shouldn't even be calling the handler via the context object. We should be creating our fake context object, then creating a new instance of the handler class ourselves, passing it $params, and then testing it by passing the context object into it and checking what we get back. And we probably need 2-3 test methods, one for each place that we could get a path from in getValue(). http:query:q is but one of them. :-)

This actually goes much father than I had anticipated! I didn't expect you to go through and kill $_GET['q'] yet. :-) That's fine, except that all of core just moved around. I'm praying that git will magically take care of that when we next merge from the core 8.x branch. I'll sort that out, probably after this gets merged.

The System path handler kinda sucks right now, but as noted above there's not much we can do about that given Drupal today. We'll accept that as is for now.

Also, I don't know why you made a new branch, and especially why you started over on it. That actually eliminates the history that would have included the work in the 1262014-context-request-path-handling branch. It would have been better to just keep working there. It's probably more trouble than it's worth to try and clean that up at this point, but note for future reference.

Thanks, aspilicious! If you can address the issues above then we're probably closing in on a submittable patch, as we'll have 3 handlers implemented and one core WTF fixed as a result. :-)

1)
I prefer functions like

request_path() and current_path() over
drupal_get_context()->getValue('path:raw')
and
drupal_get_context()->getValue('path:system')

Why else did we introduce current_path() in the past? :)
It's the same for request_uri(). That one will probably be http:request_uri in the future

In stead of marking this deprecated I would group those in path.inc.
It's also easier for the api to grab those functions.
And if all those $_GET['q'] calls used current_path() I only had to change one instance and not 100
Even for this patch I'm even not sure why we don't use current_path() in all these lines. They all get called after full bootstrap else path:system would fail.

Asked in irc and they prefer the wrappers for dx

2) +use \Drupal\Context\Handler;

That was alrdy in the http handler, so I also corrected it there.

3)

Thinking about it, actually, we shouldn't even be calling the handler via the context object. We should be creating our fake context object, then creating a new instance of the handler class ourselves, passing it $params, and then testing it by passing the context object into it and checking what we get back. And we probably need 2-3 test methods, one for each place that we could get a path from in getValue(). http:query:q is but one of them. :-)

Yeah that was my plan but I failed at implementing it :). Needs some help with that.

4)
Srry for the branch stuff...
Noted for the future.

5) When looking at the docblock of current_path() I almost had a panic attack. It said you can't use current_path() in hook_boot(), you should use $_GET['q'] in stead. As of now this will throw errors, you have to use path:system or path:raw.

Path:system uses functions not loaded in hook_boot so you can't use that. But luckily we can use path:raw instead. I changed that docblock accordingly.

6) New patched attached

Uploading patch with a working testset

Major problem:

Foreign language aliasing can work like this:

site.com/fr/alias
site.com/en/alias

How do they do this? Well they take $_GET['q'] split off the language prefix and save the last part back into $_GET['q'].
As I can't "edit" context I can't use path:raw for this. So I don't touch it.

Ok it works now BUT it's very fragile. Why? This mechanism gets called in: DRUPAL_BOOTSTRAP_LANGUAGE
This means we cannot call path:raw or path:system before this phase else fr/alias will get cached as the path:raw.
It's not a "big" problem but it show how fragile the bootstrapping code is...

Current code:

      // $_GET['q'] might not be available at this time, because
      // path initialization runs after the language bootstrap phase.
      list($language, $_GET['q']) = language_url_split_prefix(isset($_GET['q']) ? $_GET['q'] : NULL, $languages)

So reverting my changes on this piece of code made the tests work again.

Bootstrap--

Let's skip that for now and let Gabor and plach deal with it when we get to figuring out language context. :-)

Turns out the tests were failing in this issue due to #1297142: Remove stack logic complexity from the Context object business functions. I fixed that up, merged it, and then merged that into this branch. Now the tests pass. Attaching a revised patch for review purposes.

I decided to go ahead and merge this branch with #29. It's close enough; we can do additional cleanup later. Awesome work, aspilicious! Now we have something that the rest of core will notice. :-)

+++ b/modules/simpletest/tests/context.testundefined
@@ -575,3 +575,72 @@ class ContextHTTPTestCase extends ContextTestCases {
+  ¶

We should remove this trailing whitespace

Still fixed, this is an issue followup

Small followup

Two other followup patches with core fixes (that have not been committed yet)

Created new issues, marking fixed again.

Per catch, and blessed by Larry, moving this and all other WSCCI issues to the Drupal core queue.