Problem/Motivation

We've got this shiny new request object from Symfony2, let's use it!

Proposed resolution

OK, baby steps...

The attached patch begins the process of replacing our legacy scattered-functions with a proper request object. It is not a final version of what we want, but it's an incremental improvement on what we have now.

- Extends the Symfony Request class to a Drupal request class.
- Moves several of our existing random functions into methods in the request class, specifically current_path(), request_path(), and arg().
- Provides real unit tests for requestPath() and pathElement().
- Introduces a trivial factory function, request(), to make the request object accessible.
- Replaces current_path(), request_path(), arg(), and ip_address() with trivial calls to request().
- Marks request(), current_path(), request_path(), arg(), and ip_address() as deprecated.

Note: This patch requires the RTBC namespace reorganization in #1400748: Proposal for unified namespace organization. I've attached it twice, once including that patch (for testbot) and once without (for review purposes). Presumably that one will be committed first.

Remaining tasks

Once this is in:

- Someone needs to go expunge all use of current_path(), request_path(), arg(), and ip_address() in favor of the direct methods on the request object. Yes, that means some lucky developer will be able to go down in history as the person who killed arg().
- The path lookup system needs to be OOPified and made properly unit testable. We cannot provide real unit tests of the systemPath() method until that's done, because it depends on the path aliasing system. I am tempted to say that we need to move systemPath() out of being a method because of that, but I am punting on that question until we have a proper dependency injection system in core.
- The request() function is not final and will not be staying through to code freeze. It's just there for now since we don't yet have a proper injection system. Once we do, it will be removed in favor of a larger refactoring. But at least we can kill arg().

User interface changes

None.

API changes

current_path(), request_path(), arg(), and ip_address() are now deprecated and should be removed in later patches in favor of the new methods on the request object.

Note: The patch is not quite ready, sorry. As I was writing this I realized there were a few places I needed to exterminate arg() yet just to make things work, which is taking past my bed time. Stand by.

Files: 
CommentFileSizeAuthor
#75 start_using_request_object-1417320-75.patch91.6 KBandremolnar
FAILED: [[SimpleTest]]: [MySQL] 34,407 pass(es), 53 fail(s), and 5 exception(s).
[ View ]
#72 start_using_request_object-1417320-72.patch88.78 KBandremolnar
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#69 start_using_request_object-1417320-69.patch49.89 KBandremolnar
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#61 1417320-request-object.patch29.52 KBCrell
FAILED: [[SimpleTest]]: [MySQL] 34,230 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#59 request_path-1417320-59.patch27.5 KBplach
PASSED: [[SimpleTest]]: [MySQL] 34,081 pass(es).
[ View ]
#51 1417320-51.patch27.6 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 34,031 pass(es).
[ View ]
#43 1417320-43.patch27.27 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 34,032 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#40 1417320-40.patch25.22 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 34,022 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#38 1417320-36.patch23.42 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 34,018 pass(es), 11 fail(s), and 0 exception(es).
[ View ]
#36 1417320-36.patch23.42 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1417320-36.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#35 1417320-35.patch22.59 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 33,908 pass(es), 17 fail(s), and 0 exception(es).
[ View ]
#32 1417320-32.patch22.5 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 33,907 pass(es), 22 fail(s), and 1 exception(es).
[ View ]
#29 1417320-29.patch21.74 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 33,844 pass(es), 97 fail(s), and 22 exception(es).
[ View ]
#27 1417320-request-object.patch21.42 KBCrell
FAILED: [[SimpleTest]]: [MySQL] 33,837 pass(es), 129 fail(s), and 28 exception(es).
[ View ]
#24 1417320-request-object.patch98.95 KBCrell
FAILED: [[SimpleTest]]: [MySQL] 33,240 pass(es), 128 fail(s), and 28 exception(es).
[ View ]
#20 1417320-request-object-18.patch15.48 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#18 1417320-request-object.patch99.38 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#16 1417320-request-object.patch24.29 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#13 1417320-13.patch20.91 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 33,623 pass(es), 310 fail(s), and 47 exception(es).
[ View ]
#6 1417320-request-object.patch20.59 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#4 1417320-request-object-review.patch20.4 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 1417320-request-object-test.patch49.67 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#1 1417320-request-object-review.patch19.83 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#1 1417320-request-object-test.patch49.1 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new49.1 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
new19.83 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

And now patches!

I also introduced another method, pathElements(), because arg() was apparently being used without parameters in a number of places as an alias for explode, effectively. Yes, arg() is that evil of a function.

I am actually very tempted to mark pathElement() and pathElements() deprecated, as they're still only slightly less evil than arg().

Status:Needs review» Needs work

The last submitted patch, 1417320-request-object-test.patch, failed testing.

Simpletest failed to enable? How? It enables just fine for me, and I was able to run the new tests locally. Enabling other modules works, too.

Status:Needs work» Needs review
StatusFileSize
new49.67 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
new20.4 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

OK, let's try again...

Status:Needs review» Needs work

The last submitted patch, 1417320-request-object-test.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.59 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

How about this one, bot?

Status:Needs review» Needs work

The last submitted patch, 1417320-request-object.patch, failed testing.

Component:base system» wscci
Status:Needs work» Needs review
Issue tags:+WSCCI

Unfortunately this is blocked hard by: #1420530: "failed to enable simpletest module" testing a patch. The code runs fine for me locally, but testbot is breaking for unknown testbot reasons.

This could still use human review in the meantime while testbot gets unborked, so setting back to CNR.

Also refiling to WSCCI for organizational purposes.

My observation on the problem is: the testbot runs with clean URLs on and this patch breaks clean URLs. When I load 'checkout/user' I get 'Welcome to checkout No front page content has been created yet.' and no user form in sight. It's there on ?q=user but that's not what the testbot requests. It requests checkout/user.

Edit: Dave Reid points out the tests run with clean URLs off but that's simpletest not setting the clean_url variable, this is the host running pifr and THAT is using clean URLs. So you need 'user' and 'admin/modules' working...

Edit2: so you have three Drupals to debug, the testbot, the checkout and the simpletest-installed Drupal. And if sun succeds with the git-based update tests #1364798: Impossible to generate meaningful diffs of upgrade db dumps then for those they will be different major versions: D6, D8, D7! Awesome. This will be so much to fun to debug.

chx++

D'Oh! Thanks, chx. That's something I did not check locally. I'll try to revisit that tonight and see what I borked.

@Crell: here's a quick hack to make clean urls work:

line 437 in menu.inc, change:

<?php
    $path
= $_GET['q'];
?>

to
<?php
    $path
= request_path();
?>

That makes a request to any path with clean urls enabled work again. Of course, this probably not the right place todo this, but it might help to at least get the patch running :)

StatusFileSize
new20.91 KB
FAILED: [[SimpleTest]]: [MySQL] 33,623 pass(es), 310 fail(s), and 47 exception(es).
[ View ]

Ok, just for the sake of testing, let's see of that change in menu_get_item makes the tests run.

Status:Needs review» Needs work

The last submitted patch, 1417320-13.patch, failed testing.

Well it does get as far as running now! :-) Thanks. Looks like I've got a bit more work to do...

Status:Needs work» Needs review
StatusFileSize
new24.29 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

OK bot, what do you think of this?

Status:Needs review» Needs work

The last submitted patch, 1417320-request-object.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new99.38 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

Let's back up a bit...

Status:Needs review» Needs work

The last submitted patch, 1417320-request-object.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new15.48 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Fixed RewriteBase in .htaccess. :)

Status:Needs review» Needs work
Issue tags:-WSCCI

The last submitted patch, 1417320-request-object-18.patch, failed testing.

Status:Needs work» Needs review

#20: 1417320-request-object-18.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+WSCCI

The last submitted patch, 1417320-request-object-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new98.95 KB
FAILED: [[SimpleTest]]: [MySQL] 33,240 pass(es), 128 fail(s), and 28 exception(es).
[ View ]

Grrr... Bad Git!

[Thu Feb 02 04:16:22 2012] [error] [client 10.20.0.109] PHP Fatal error:  Class 'Drupal\\Core\\Request' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc on line 2731
[Thu Feb 02 04:16:37 2012] [error] [client 10.20.0.1] PHP Fatal error:  Class 'Drupal\\Core\\Request' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc on line 2731

Crosspost ... this caused the failure in #20.

Status:Needs review» Needs work

The last submitted patch, 1417320-request-object.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new21.42 KB
FAILED: [[SimpleTest]]: [MySQL] 33,837 pass(es), 129 fail(s), and 28 exception(es).
[ View ]

That was a bad patch anyway. Damn you git!

Status:Needs review» Needs work

The last submitted patch, 1417320-request-object.patch, failed testing.

StatusFileSize
new21.74 KB
FAILED: [[SimpleTest]]: [MySQL] 33,844 pass(es), 97 fail(s), and 22 exception(es).
[ View ]

Ok, the reason why a request to 'user' now fails is because menu_set_active_item() (called in user_page() in user.pages.inc for an authenticated user) overwrites $_GET['q], but this isn't reflected in the systemPath variable.

This patch adds a setSystemPath() method so we can overwrite this. Attaching the new patch to see how many fails we have now - note, I'm not sure if this is the right way to go of course.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1417320-29.patch, failed testing.

StatusFileSize
new22.5 KB
FAILED: [[SimpleTest]]: [MySQL] 33,907 pass(es), 22 fail(s), and 1 exception(es).
[ View ]

Ok, now lot of of fails are re: multilingual sites with language prefix which fail completely resulting in a page not found. Using the same technique in locale_language_from_url() function as in previous patch with the setSystemPath() method. Let's see how many fails now :)

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1417320-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new22.59 KB
FAILED: [[SimpleTest]]: [MySQL] 33,908 pass(es), 17 fail(s), and 0 exception(es).
[ View ]

This should at least make a few more passes. The change is in path.inc in drupal_is_front_page() (line 295) (man that $_GET['q'] is a real killer)

Debugging now on other failures.

StatusFileSize
new23.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1417320-36.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Got some more passes, this time, drupal_path_initialize() is completely 'requested' (humor, right haha - i've been debugging to much I guess).
Also fixed a redirection in bug in drupal_redirect_form() which will make some more passes, let's see.

Thanks, swentel!

I spoke with Dries this morning about this patch. To be clear, modifying $_GET['q'] is a critical bug in Drupal, and the request object in Drupal 8 should *not* replicate that bug. That is, setSystemPath() may not appear in the final Drupal 8 release. If we have to refactor other parts of Drupal heavily in order to achieve that... well, then we're doing it right and eliminating code debt. :-)

So *for right now* we can include that sort of hack, but the method *must* be documented as deprecated from the get-go, and the docblock for it should be marked "never use this or the most adorable kitten you know will die a slow and painful death" (or something). I will also be opening a critical to remove such BC hacks, which will be a D8-release blocker.

Modest steps to eliminate some needs for that in this patch are fine, although if they're too difficult we can punt to later smaller patches.

That said, carry on. :-)

StatusFileSize
new23.42 KB
FAILED: [[SimpleTest]]: [MySQL] 34,018 pass(es), 11 fail(s), and 0 exception(es).
[ View ]

Reuploading the patch from #36 to see how many fails left - it somehow didn't started (or the testbot died). Debugging goes on tomorrow.

I also agree that the setSystemPath() is not really nice. I think a couple of the 'fixes' I did with it can be solved, but the language negotiation part requires a separate issue - and some discussion with Gabor. I'll just try to get this patch green first and see what I can do after that. Either way, it will show us where we need to start refactoring first :)

Status:Needs review» Needs work

The last submitted patch, 1417320-36.patch, failed testing.

StatusFileSize
new25.22 KB
FAILED: [[SimpleTest]]: [MySQL] 34,022 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

ok, we should be down to 7 fails now.

Regarding the IP address and HTTP_HOST, I'm not sure what todo here. The reason they fail is simple:
- first fail is because the drupal_static_reset doesn't work as ip_address() doesn't use drupal_static anymore. That one can be solved with a request()->server->set() to override the $_SERVER variable
- the 3 remaining fails are because the symphony getClientIP method doesn't have the notion of reverse_proxy_addresses: are we just going to drop this feature or not ? If not, we'd have to either just keep ip_address or create or own method.

Uploading to see if it's really 7 fails. I've added a drupal_goto() in user_page() instead of the setSystemPath() function which makes breadcrumbs and session tests pass. The translation pass is because of a change in locale_block_view()

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1417320-40.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new27.27 KB
FAILED: [[SimpleTest]]: [MySQL] 34,032 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Ok, this should turn green!

- changed a test in menu that asserts a user is redirected to user/uid when going to user/login (due to my change user user_page() this test suddenly failed)
- theme_links unit tests assumed a $_GET['q'] - so I've used the evil setSystemPath() - we could convert this test to a webTestCase though
- I've reverted the ip_address() back to its original state because the Symphony getClientIP doesn't support a custom reverse proxy header. I'll leave it up to you guys what you want todo with this.

Status:Needs review» Needs work

The last submitted patch, 1417320-43.patch, failed testing.

I've added a drupal_goto() in user_page() instead of the setSystemPath() function which makes breadcrumbs and session tests pass.

That sounds like we're losing a feature and introducing a performance regression?

That sounds like we're losing a feature and introducing a performance regression?

Internal forwarding was really disturbing, and I fought with it more than once in my life: I'd be happy to have the right redirect directly after the login post instead of having this polymorphic "user" path that actually has an almost unpredictable behavior and seriously messes up urls by creating duplicates for logged in user. If the right redirect right after login is done, then there is no performance regression, and the redirect on 'user' would just happen in case of accidental user path hit.

EDIT: We are actually loosing a bug and creating a SEO and consistency improvement.

It's not only the /user path that does this. Comment permalinks in D7 do a similar thing, where the functionality is much more important (because if we did a redirect, it may not be accurate over time given paging, threading etc.).

Also this isn't just 'accidentlal user path hit'. Part of the reason the behaviour of /user is like this is so there can be a single menu item which works for both login and the user account depending on status. A menu link can only point to one path, so it'd actually be any time someone clicks on the link to their user account from that menu.

And what is so bad about having two menu links instead? It'd much more explicit, and that doesn't create any UX issues since menu items are displayed only after check access is done. It also would allow natively people to put those two menu entries at different places (which definitely makes sense).

The polymorphic links are a usability, SEO, and maintainability issue.

And there's no SEO improvement for something that primarily affects logged in users. If there really is duplicate content somewhere, there is always rel canonical.

You said that comments permalinks does the same thing: it's not for logged in users only. We should get rid of polymorphic menu entries.

StatusFileSize
new27.6 KB
PASSED: [[SimpleTest]]: [MySQL] 34,031 pass(es).
[ View ]

Ok, this one should be green!

@catch Well, this would be interesting for authenticated caching ;) No seriously, I have no idea whatsoever how to fix it without losing the feature. It should turn green now, then we can identify what next steps we should take, because this patch is obviously not ready at all :)

Status:Needs work» Needs review

Status:Needs review» Needs work

NICE! Notes from a cursory review:

+++ b/core/includes/bootstrap.incundefined
@@ -2875,8 +2802,7 @@ function ip_address() {
-  return $ip_address;
-}
+  return $ip_address;}

Woops, looks like you moved the closing bracket by accident.

I wonder if some of those explode('/', request()->systemPath()) just be calls to pathElements()?

I could live with two menu items, that seems fine in terms of implementation compared to what we have now or the redirect.

If it's only user_page() that's affected and not functions like comment_permalink() then we might be OK making the change like that, but given user_page() is only using two menu API functions (not even directly changing $_GET['q']) let's be sure we know exactly what we're losing before it's gone.

Fair enough, it maybe worth an issue only for introspecting this problem.

I'm still working on the assumption that we'll be overhauling the logic in those places anyway. At this point the goal is just to get a request object in place that we can use as a tool to unravel the spaghetti that forces us to mess with $_GET['q'] in the first place. So I'm fine with switching to a redirect and revisiting that later once the underlying tools are better.

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+  /**
+   * Set the systemPath().
+   *
+   * @deprecated
+   *
+   * Never ever use this or a 1000 kittens will
+   * hide under your bed, scream all night and haunt your dreams.
+   */

LOL! +1!

I'm disapointed, it does not mention any ponies.

Status:Needs work» Needs review
StatusFileSize
new27.5 KB
PASSED: [[SimpleTest]]: [MySQL] 34,081 pass(es).
[ View ]

Had a quick look to the patch and (again) could not find anything wrong with it aside from some minor coding standard issues. Rerolled the patch with those and #53 fixed.

Ponies still missing :(

+++ b/core/includes/bootstrap.inc
@@ -2725,124 +2720,56 @@ function language_default() {
+ *   The request object for this request, as populated from the PHP superglobals.

Comment is past column 80. I removed the comma.

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+   * The requested URL path of the page being viewed

Missing trailing dot.

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+   * index 0 returns "admin", index 1 returns "structure", and index 2 returns

"index" should be moved one line up.

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+   * - http://example.com/path/alias (which is a path alias for node/306) returns

Comment is past column 80.

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+   * - http://example.com/index.php returns an empty string (meaning: front page).

idem

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+        // This is a request with a ?q=foo/bar query string. That trumps all other
+        // path locations.

idem

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+        // Unescape and strip $base_path prefix, leaving q without a leading slash.

idem

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+        // If the path equals the script filename, either because 'index.php' was

idem

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+      // Under certain conditions Apache's RewriteRule directive prepends the value
+      // assigned to $_GET['q'] with a slash. Moreover we can always have a trailing

idem

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+   * - http://example.com/path/alias (which is a path alias for node/306) returns

idem

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+   * instead.  Be aware that requestPath() does not have aliases resolved.

Surplus space.

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+      // including writing back to it at times.  Those are all critical bugs,

idem

+++ b/core/lib/Drupal/Core/Request.php
@@ -0,0 +1,175 @@
+   * Never ever use this or a 1000 kittens will
+   * hide under your bed, scream all night and haunt your dreams.

LOL

I think this is allowed to wrap its own way.

+++ b/core/modules/simpletest/tests/request.test
@@ -0,0 +1,92 @@
+    // This unfortunately necessary because Drupal's registry throws a database

Missing 'is'

+++ b/core/modules/simpletest/tests/request.test
@@ -0,0 +1,92 @@
+  // @TODO: Add a unit test here once the path logic becomes an object and can

Normally we use the lowercase form: @todo.

24 days to next Drupal core point release.

Anything that says @deprecated should explicitly mention what it's deprecated in favour of. It's good to be able to do API changes like this and leave BC layers in temporarily, but if they're actually temporary as opposed to "@todo I don't like this let's get rid of it", then it needs to be extra clear what the change is going to be - we never know when the followups for these will actually land.

There's some annoyances in here like not using arg() but then still assigning the result to variables called $args that could use cleaning up, and some of the rhetoric in the comments is overblown, but the actual code generally looks fine to me.

StatusFileSize
new29.52 KB
FAILED: [[SimpleTest]]: [MySQL] 34,230 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

New patch. Only changes from #59 should be chasing 8.x (that doesn't sound anywhere near as good as chasing HEAD), and updating documentation around things that are deprecated per #60.

Status:Needs review» Needs work

The last submitted patch, 1417320-request-object.patch, failed testing.

This is just a detail, but I would rename requestPath() systemPath() pathElement() and pathElements() methods as getRequestPath() getSystemPath() getPathElement() and getPathElements() in order to be consistent with the Symfony\Component\HttpFoundation\Request naming convention, which already embeds methods such as getBasePath() and getBaseUrl(). Using this the same naming convention (i.e. using get prefix here) would make its usage much more obvious, for developers, especially when using IDE's capable of autocompletion.

As I said on IRC, the getters in the Request method are doing the initialization logic again and again when the result is an empty string or an empty array (for instance when you end with requestPath being an empty string). You change change the if (empty($this->requestPath)){ check to if (null !== $this->requestPath){

Btw, I agree with @pounard for the getter names. Prefixing them with get would follow the coding standards used by most PHP frameworks out there.

Since this patch is baby steps I don't want to introduce scope creep on this patch, but I'm wondering if I should update this patch with other places where the request object comes into play in this issue or open new issues?

e.g. in bootstrap all reference to $_SERVER could be replaced with $request()->server->get/set/has

Title:Start using the request objectIntroduce the Request object

The way forward is to introduce the Request/Response objects in core, and primarily focusing on that.

There's a patch for this here already, but it approaches the topic from a "scope-creep perspective" (sorry). Given the plethora of discussions we had at the WSCCI sprint, I'd like to see this issue to start from scratch and introduce the Request object at the heart - menu_execute_active_handler().

What's scope creepy about the patch now?

Instead of the primary objective, the existing patch tries to attack the secondary side-benefit; i.e., removing other globals and whatnot - but that's not the point of introducing the Request object.

(Note: I want to write a very longish blog post about this on the weekend, so comments like this are totally too sparse)

How can you introduce the request object and not remove usage of all the request globals?

The primary objective of introducing a Request object is more or less to instantiate it right within or after routing happened, to populate it with the loaded router arguments, and pass that information forward to the responsible callbacks.

Can't we handle the complain about the request object being global in follow-up issues?

Eliminating globals, cleaning up whatever mess, is not the primary objective. These are secondary benefits we gain from a global Request object, not the primary objective.

To clarify, I do not complain about the Request object itself being global. It's the approach of "let's eliminate some others globals, because we can" before actually introducing the Request object properly, and passing it down the callback chain, which makes up the scope-creep. First things first.

Exactly this primary objective actually is a hard dependency for the newborn Block/Layout effort, since every block plugin potentially needs to get that Request object passed in.

In fact, I already see myself coding a prototype that showcases what I mean with "focusing on the primary objective" as I've the impression that only a few people seem to understand what I'm after...

That said, we already discussed at the sprint that most globals can probably be hacked-in (for the time being, and if necessary at all in the first place) by simply squeezing a reference to them into the Request object. But again, eliminating our global variables is not the central point of a Request object. (especially, since that object [also Symfony's] is just a dumb container). Attacking this whole thing from a "let's eliminate some global variables" perspective is the wrong approach. No bang for the bucks.

The main point is the primary architectural change, which means that we setup the Request object in the router system, and pass it forward to the invoked callbacks (if they support it [and Symfony actually provides a solution for this]). The callbacks may pass it forward to other functions they call on their own.

Implement that, plus a few examples to demonstrate how it's going to look like, and to prove that it works — that's the primary objective.

sun: The problem is that once we start using the request object, all of those other places we have random crap scattered around will break, too. Or rather, will be out of sync with the request object, and cause all sorts of weird bugs. We cannot and will not be writing into the request object to mess around with its equivalent of $_GET['q'], yet we write back to $_GET['q'] in several places in core now. That has to stop. The spaghetti is sadly quite deep. :-(

While you insist that eliminating globals is not the goal, I'd argue that it is an intrinsic part of the goal. We simply cannot do what we want to do with subrequests and so forth unless the globals are eliminated. So whether we attack the problem from a "reduce globals using the request object" perspective or a "put the request object in place and then use it instead of the globals" perspective, the end result is the same: A request object that we use for all of this incoming stuff instead of throwing $_GET and $_POST around everywhere.

Looking at this code and coming off of reading #66 I'm curious why things are being left so 'late' in the code. e.g. why is drupal_classloader being called so 'late' in the early early bootstrap? why would you defer the instantiation of the request object? (I have plenty of other questions too - but I'll stay as close to on topic here as I can)

The primary objective of introducing a Request object is more or less to instantiate it right within or after routing happened, to populate it with the loaded router arguments, and pass that information forward to the responsible callbacks.

I'm not sure I follow this at all. The request object is the container for all the http request information - I would think that you'd want to have this available ASAP - i.e. at the very earliest stages of bootstrap.

There is a lot of superglobal request fiddling (checking/reading/updating) that Drupal is doing in the early early bootstrap. Why wouldn't we want to take advantage of the methods available to us in the request object?

After #65 I started replacing some of the uses of $_SERVER with request()->server to maybe roll a patch. Besides having to fix problems caused by a late late call to drupal_classloader, I've have yet to come across ill effects and there are places where the code is looking a bit more tidy with request available from the very begging of the bootstrap.

I suppose I'm going to have to catch up on a lot more of the discussions that have happened thus far (I was actually trying to hunt down any posts about the most recent wscci sprint earlier today - ironically at the same moment Dries was posting his blog). So please do forgive me if these questions have been talked to death - and please be kind enough to point me to the appropriate discussions.

StatusFileSize
new49.89 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I went ahead and followed up on #61 and took things a bit further. This patch is here as work in progress to get some feedback.

I have a few @TODO items where I'd have to think a bit more about how/whether the request object could be used to replace some existing code.

I fixed the tests that would have broken with this patch, but didn't look at the failed tests from #61 - so I suspect there will be 4 failures from testbot.

What I did here was take any code that was directly reading or manipulating $_SERVER and use methods from the request->server object. In some cases I replaced some code with methods from the main request object.

This patch also instantiates the loader sooner as per http://drupal.org/node/1290658#comment-5614058 - otherwise I wouldn't have had access to the request object in earlier parts of the bootstrap.

Status:Needs work» Needs review

Updating status.

Also going to note that as a work in progress there are some parts of this code that could use a bit more refactoring. Still I'd love feedback before I move forward.

Status:Needs review» Needs work

The last submitted patch, start_using_request_object-1417320-69.patch, failed testing.

StatusFileSize
new88.78 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Okay #69 is messed up - I didn't actually include the request class and related tests in the patch (forgot to add/commit them to my repo). I also discovered that a bunch of other tests were broken since the $_SERVER variable wasn't being manipulated directly in bootstrap - so request object had to be in the tests too. That's not a bad thing.

As before there are a few places that could use some refactoring since the request object does some of the things that are happening in code anyway. So there are a couple of @TODOs for second looks.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, start_using_request_object-1417320-72.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new91.6 KB
FAILED: [[SimpleTest]]: [MySQL] 34,407 pass(es), 53 fail(s), and 5 exception(s).
[ View ]

Lets try this one more time.
It seems that I didn't have the request object available in the install file and it was causing test failures because of the late loading of the autoloader. This patch includes http://drupal.org/node/1290658#comment-5628464 to solve that problem.

Status:Needs review» Needs work

The last submitted patch, start_using_request_object-1417320-75.patch, failed testing.

Status:Needs work» Needs review

I have another patch that fixes most of the errors found by testbot - but after talking to @crell in IRC it seems my patches are out of scope for now (at least on this issue). While it does start us down the path of eliminating direct access to Superglobals I'm okay waiting to introduce these changes once more decisions have been made or at least until we actually have the request object committed and available.

Please feel free to review the patches to see what code looks like without mention of $_SERVER though - and I welcome feedback on the @TODO comments I left in the patches.

Status:Needs review» Closed (duplicate)