![Announcement icon Announcement icon](https://www.drupal.org/files/styles/grid-2-2x-square/public/announcements/drupal-evergreen-logo-280X280px%20%281%29_0.jpg?itok=PHpn6rCb)
2 years ago, we changed Drupal's .htaccess URL rewrite rules such that clean URLs do not internally map to a 'q' query string parameter (#284899-147: Drupal url problem with clean urls). Meanwhile, all modern web servers can route URLs like http://example.com/index.php/foo to index.php, since that is part of the CGI specification (http://www.ietf.org/rfc/rfc3875 section 3.2). Given that both clean and dirty URLs can work without Drupal's legacy "q=" pattern, it is a WTF that we still have so much code in Drupal that both sets and reads PHP's $_GET['q'] variable. Furthermore, this gets in the way of current work to refactor Drupal's request routing logic to leverage Symfony classes (e.g., #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel), both because Symfony's Request class initializes its path from the URL path (not $_GET['q']), and because we can't reroute the reading of $_GET['q'] to a method call on an appropriate $request object (whereas if all current Drupal code were to call a function like current_path() instead, that function could be progressively iterated to integrate with the appropriate object). Therefore, this issue aims to remove all legacy handling of 'q' as a special query string parameter, and uses current_path() and friends for all path related logic.
Doing this also provides the opportunity to remove 'clean_url' as a configuration variable. If the only difference between clean and dirty URLs is a leading 'index.php/', then we can treat that similarly to how we treat the global $base_url / $base_path variables: just like we autodetect if we're serving the 'foo' path at the URL http://example.com/subdirectory/foo rather than at http://example.com/foo, and adjust all URLs accordingly to be relative to 'subdirectory', we can do the same for 'index.php'. The logic is a little different and requires another global variable $script_path, rather than simply appending to $base_path, because URLs to files (e.g., css, js, images, etc.) need to be relative to $base_path while URLs to Drupal paths need to be relative to $base_path . $script_path, but other than that, the essentials of working with relative URLs are the same.
Prior summary that focused on the problems of a 'clean_url' configuration variable
Right now we enable clean URLs in the installer if we can, but then have a bizarre admin screen for checking and enabling the feature, as well as many, many weird hacks in core to support non-clean urls. Image derivative callbacks are one.
Instead of this, we could try to get a more robust clean url check in the installer itself (probably a dedicated .php file so it doesn't require the request to Drupal itself and a full bootstrap, which has also led to a tonne of hacks in the installer), then point people to how to get it enabled if it's not.
Clean URLs should work on 99.9% of web hosts, so it is really going to be people who are running their own servers who run into the issue, and if they're going to run a normal Drupal site with clean URL support they need to fix it anyway.
drush installs etc. are not going to be able to check, but if clean url support works then the site will anyway - and we won't have a crappy variable_get('clean-url') that can go out of sync any more.
Follow Ups
- #1546082: Follow-up to variable_get('clean_url') removal
- #1515196: Standardize retrieval of "global" variables to be at beginning of function
- #1540390: Deprecate the drupal.sh script
- #335411: Switch to Symfony2-based session handling
Upgrade issues
What about sites upgrading from D7 and earlier that have legacy content containing links to http://example.com/index.php?q=foo, as well as other sites / search engines / users with bookmarks who have those links? The suggestion in #29 is to require the site to implement a redirect solution, possibly with the help of a contrib module.
Comment | File | Size | Author |
---|---|---|---|
#94 | 1183208-drupal-q-94.patch | 114.24 KB | no_commit_credit |
#94 | interdiff-1183208.patch | 829 bytes | no_commit_credit |
#88 | 1183208-drupal-q-88.patch | 114.21 KB | effulgentsia |
#88 | 1183208-drupal-q-88-interdiff.txt | 4.88 KB | effulgentsia |
#73 | phpinfo.png | 54.87 KB | cosmicdreams |
![Acquia logo](/files/ads/acquia-issue-sponsorship.png)
Comments
Comment #1
catchComment #2
cweagans+1000000. And subscribe.
Comment #3
joachim CreditAttribution: joachim commentedAgreed. Using dirty URLs on a commercial project is completely unthinkable, and the only time I ran into trouble was on a server that was running Zeus web server -- and even then, I got it working.
Passing thought: could support for dirty URLs be taken care of in contrib for those that really need it?
Comment #4
podarokSubscribe
possibly we need to provide docs for such servers like nginx... or included contrib module for them...
nginx can do clean urls nut completly non via .htaccess
Comment #5
Crell CreditAttribution: Crell commentedSince we know we're switching our core HTTP handling to Symfony, I should note that Symfony does not use ?q=foo for its routing. It uses index.php/foo. That seems to work out of the box with no modifications, and then you can mod_rewrite it back to normal. But, I think it handles it automatically anyway without a toggle.
So we could drop "dirty URL" support as a matter of course when we drop in the new kernel. :-)
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedOpened #1474018: Remove all traces of old-style dirty URLs, $_GET['q'], and 'clean_url' variable to do this in the WSCCI sandbox. The approach taken there is to still allow people to use /index.php/foo instead of /foo, and to make url() choose whether to output the index.php/ part based on whether the page was requested with it. And therefore, remove 'clean_url' as a config variable. Still a work in progress, but please comment there if you like.
Comment #7
catchThis sounds like a good plan to me, if we can remove the variable that'd be great.
I have a very old site that was migrated from an even older site that has one path alias (I thought it was more, but it looks like only one now) with index.php in the alias to match the old CMS. The same could be true for redirects. However I don't think there's a problem requiring clean urls to make that work, it just flashed through my mind briefly but let's forget it was ever mentioned.
Comment #8
MichelleNot necessarily. In my case, I haven't managed to get clean URLs working on my local dev environment but they work fine on my (managed) VPS.
This is an edge case and I don't expect it to be a blocker but wanted to add $.02 from someone who is fairly clueless when it comes to server stuff since the folks on this issue are all people who are unlikely to ever not be able to get clean URLs going and may not realize this could cause some people problems.
Comment #9
cweagansIn my experience, it's been because somebody doesn't have the apache rewrite module enabled or because AllowOverride None is set for their docroot. However, that's only really a concern if you're actually setting up Apache. Most people use a package like Mamp or Xampp or similar bundles and don't really have to configure that kind of thing.
In other words, let's document the requirements (which I think is already done) and direct people to the various support channels if they still can't get it working.
Comment #10
catchIt sounds like if we go with effulgentsia's idea, then we'd be able to drop the configuration option, while still having at least some kind of fallback for when the server doesn't support it. That could be best of both. The main places it is annoying to support are the config option and the various switches in url(), overlay, image derivatives etc. which all have to account for it separately - if we can drop that requirement then there's no need to drop the support so seems well worth trying that first.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedThis patch:
This still has variable_get('clean_url') calls for file module, image module, and JS/CSS gz creation. I'm not sure how to handle those yet, but wanted to get this up for a bot check and human review in the meantime.
Comment #12
sunWhat a frickin' AWESOME patch is that?! :-)
This comment should be kept.
I don't think we want to remove this option. It allows code (and especially tests) to link to URLs that are delivered by other main application scripts than index.php.
We're merely changing its semantics - the "non-clean URLs" case just turns from update.php?q=foo into update.php/foo
First impression on this function is: "odd." ;)
1) We don't separate between front and back.
2) It doesn't really return a "path."
3) The name is very long for a potentially often used helper that returns something short.
Let's think about alternative names. :)
That's "a bit" too compact ;)
Comment #14
Crell CreditAttribution: Crell commentedWow, OK, this is substantially more viable than I anticipated.
"Front controller" is a common architectural pattern used by nearly all modern applications, including Drupal. It's entirely descriptive. I don't mind it being a longer function name since few people should ever be typing it.
That said, how will this change again when we get the kernel in place, since the Request object does this sort of thing already? I forget if it's UrlMatcher or something else that generates URLs, which we'll probably want to do eventually as well.
Comment #15
kika CreditAttribution: kika commentedWhat about commented-out redirect in .htaccess to provide backward-compatibility for ?q= paths? Sorry if this is answered in this patch.
Comment #16
cweagans@kika, the entire point of removing this is that it adds extra complexity and *nobody* uses dirty URLs. I don't think we need to provide an upgrade the path. The upgrade path is to fix your config.
Comment #17
webchickEr. We do need to provide an upgrade path. I could very well have links in my node bodies to "?q=node/4"
Comment #18
cweagansRight. You could. But who DOES? Really, who runs a Drupal website without clean URLs?
Again, that's the entire reason we're dropping it.
Comment #19
webchickAccording to http://www.google.ca/#hl=en&sclient=psy-ab&q=inurl:%3Fq%3Dnode%2F1&oq=in......
- http://www.integralinstitute.org/
- http://www.climateofdenial.net/
- http://www.fogproject.org/
- http://www.hdpvrcapture.com/
...and about 9 more pages of results. :P
Comment #20
tstoecklerNot sure if that really counts. See this one:
http://www.google.ca/#hl=en&sclient=psy-ab&q=site:drupal.org+inurl%3A%3F...
EDIT: Not implying that your examples don't count, I was trying to imply that maybe not all of the 9 pages actually have clean URLs enabled.
Comment #21
MichellePeople who don't know any better. People who tried once to get it working and hit a roadblock and gave up. People who don't care about SEO.
I think we need to be careful about making assumptions. Those who have the skills to write this patch are unlikely to run a site without clean URLs but that doesn't mean there aren't many out there who just don't for one reason or another. If we aren't offering an upgrade path, it needs to at least be very well documented.
Comment #22
cweagansOkay, so say we decide to provide an upgrade path. How do we do it?
Our options:
1) Use an apache rewrite. This is not an option because the people that need this weren't able to get the ORIGINAL apache rewrite working in the first place, so why would they be able to get this working?
2) Edit their content for them. This feels dirty.
3) Leave in some kind of legacy support layer thing that watches for $_GET['q'] being set and route the request accordingly. If we do this, then we've gained nothing in this issue.
Any other options?
Comment #23
klonos...move the related code to a contrib module and point people upgrading from D7 to D8 to it. After enough time will have passed and based on that project's stats we'll be able to know if our decision to drop support for dirty URLs was right or not ;)
Comment #24
Crell CreditAttribution: Crell commentedOnce we get a mechanism to register additional listeners into the kernel in place, it should be quite possible to write a contrib-based listener to check $GET['q'] and manipulate the request object as needed. So #23 is a viable option.
That said, as noted previously (I forget which of these forking issues it was noted in) there are legitimate uses for dirty URLs in development; it makes it so that you don't have to fight Git about .htaccess file changes when rolling patches or updating if you're not in /var/www. (Vhosts and subdirs both require editing .htaccess.)
Comment #25
catchThis isn't really about actually dropping clean url support now (or at least not at the moment), so I'm re-titling the issue.
#22.1 is viable for sites with legacy ?q= incoming links that since switched on mod_rewrite and want them to redirect, so seems worth doing as well. I also think it's OK to say that if you were running your site without clean URLs in 6.x or 7.x, that you either have to figure out mod_rewrite or install a contrib project or live with some outdated links.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedI still need to deal with File and Image module, but getting another bot check to see if the other tests now pass.
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedImplements feedback from #12.
I removed the function and replaced with a global variable $script_path (I know, yuck, but we already have global variables for every other component of the URL, and I'm hoping this all gets cleaned up into a Request object when we get that far). I left the term "front controller" in some code comments, but not in any code.
This patch reverts to Drupal.settings.basePath being the same as base_path(), and adds a Drupal.url() JS function for when URLs to Drupal pages are needed.
This also finishes the file, image, and css.gz parts. With any luck, bot will go green on this.
This patch also adds some new @todo comments that reviewers should evaluate.
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedI agree. A simple redirect_q.module that implements hook_init(), checks for $_GET['q'], and does a drupal_goto() would be trivial. Or, this can get added, with configuration settings, to Redirect module.
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedCalling attention to this. Maybe needs a @todo? request_uri() says that some servers won't provide a $_SERVER['REQUEST_URI']. In this case, #28 defaults to dirty URLs. Any thoughts on whether this is ok, and if not, what to do about it?
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedComment #33
effulgentsia CreditAttribution: effulgentsia commentedRe #30, note that HEAD's request_path() function appears to not support clean URLs on servers that don't provide $_SERVER['REQUEST_URI'], so this patch doesn't make that any worse from what I can tell, unless I'm missing something.
Comment #34
Crell CreditAttribution: Crell commentedSo, one thing to consider with HttpFoundation and HttpKernel is that both support a base front controller that is not index.php. That's just the typical one, but Symfony full stack uses app.php and app_dev.php, which in turn allows you to have separately configured applications all using the same code. That would be extremely useful for, say, the installer or update.php, which could be only slightly modified kernels. If we can avoid hard coding index.php and simply support a "whatever the file was" logic like Symfony does, that's a lot of additional flexibility.
I don't know whether we can pull that off in this patch just yet, but it's a direction we should be moving.
Comment #35
effulgentsia CreditAttribution: effulgentsia commentedMost of the patch already does that, both in terms of determining the global $script_path, and in terms of having $options['script'] in url(). Where this patch retains hard-coding to index.php is here:
Comment #36
effulgentsia CreditAttribution: effulgentsia commentedThis deals with #30 by commenting that section, changing that code and HEAD code that use $_SERVER['REQUEST_URI'] directly to not do that (since we have a request_uri() function for just that reason), and adding some more @todo for existing problems in request_uri().
Comment #37
effulgentsia CreditAttribution: effulgentsia commentedThis also fixes a pre-existing incorrect comment in web.config.
Comment #38
katbailey CreditAttribution: katbailey commentedThis needed a reroll due to a recent change in overlay-parent.js.
Comment #39
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedProbably #1299424: Allow one module per directory and move system tests to core/modules/system and other file moving will prevent this from applying. Other changes also prevent auto-merging.
#38: drupal.q.38.patch queued for re-testing.
Comment #41
nod_tagging.
Comment #42
Crell CreditAttribution: Crell commentedI talked with effulgentsia the other day and we concluded that this patch should go in before the kernel patch. That makes this a blocker, so bumping its priority.
Comment #43
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSplit, rebased, rerolled. Hope I lost nothing in the process.
Yay! Tests pass. Notes from manual testing:
Setup: Remove .htaccess, do a clean install.
http://localhost/drupal-base-dir/
, click the "Modules" admin link, getting redirected tohttp://localhost/drupal-base-dir/admin/modules
that is not found.Partially expected - but maybe
http://localhost/drupal-base-dir/
should redirect tohttp://localhost/drupal-base-dir/index.php/
if that doesn't add too much cruft or is at all detectable. Fine if not.http://localhost/drupal-base-dir/index.php/
, click the "Modules" admin link, getting redirected to the correct page, but the overlay doesn't open.Comment #44
effulgentsia CreditAttribution: effulgentsia commentedThis just changes 3 places left over from prior patches that were incorrectly using
baseScript
to usescriptPath
instead, which fixes the Overlay problem from #43.Re #43.1, the key is we don't want to reintroduce a 'clean_url' variable, and we want to assume clean urls unless we have a good reason not to assume that. Some possibilities:
What I'm working on next right now is reviewing the test changes in this patch. I think I was too aggressive in removing some tests, and need to add some back. I'll post another patch when I've done that. Otherwise, I think the code itself is ready, but still needs code reviews.
Comment #45
sunum. http://example.com always works, because even without mod_rewrite, on PHP-enabled Apache servers, the built-in core mod_dir is (almost?) always configured to route into index.php through the
DirectoryIndex
directive, or the recently addedFallbackResource
directive (which has the potential to make mod_rewrite entirely obsolete in the future). Renaming index.php into drupal.php or anything else would break the front controller even in that fallback scenario.Comment #46
sunahem, speaking of... #1537540: FallbackResource support (since Apache 2.2.16+ instead of mod_rewrite)
Comment #47
effulgentsia CreditAttribution: effulgentsia commentedFrom #38 to #43, robots.txt and web.config changes were accidentally removed. This brings them back. I reviewed the whole patch again, and everything looks right to me except common.test and locale.test. Those I need to study a bit more to make sure we're not removing valuable tests.
Re #45: Right. The concern raised in #43.1 is that if http://example.com is allowed to work on sites where clean URLs don't work, then with this patch, all the links on that page will be to clean URLs anyway, and those links won't work. So, on a non clean URL environment, better to somehow make http://example.com not work or redirect to http://example.com/index.php. I think a Drupal-based redirect would best be left to contrib, but I'm open to suggestions for .htaccess and web.config changes.
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedThis makes path.test and locale.test tests more robust in working whether you run tests with clean or dirty urls. I don't have these explicitly checking both clean and dirty separately, because they don't do that in HEAD, and I don't think it's necessary as long as common.test has sufficient coverage of url(). I still need to make sure it does, but I need to stop for the night and pick up again tomorrow. No need to hold up code reviews though on merely finishing up common.test. There's 100K of other changes here that need reviews :)
Comment #49
effulgentsia CreditAttribution: effulgentsia commentedAnd this restores a couple tests to common.test. So, I think this is ready, and am looking forward to someone saying otherwise, or RTBC'ing it.
Comment #53
effulgentsia CreditAttribution: effulgentsia commentedComment #54
effulgentsia CreditAttribution: effulgentsia commentedReroll to chase HEAD.
Comment #55
cosmicdreams CreditAttribution: cosmicdreams commentedI found many instances of GET['q] in drupal.sh located in the core/scripts folder. Should we update those too?
Comment #56
tnightingale CreditAttribution: tnightingale commentedWhile I'm not extremely familiar with the internals of bootstrap.inc, the patch doesn't appear to be doing anything radically different so looks good to me.
Aside from core/scripts/drupal.sh (mentioned above), it catches all instances of GET['q'].
Patched codebase installs on my local system without issue and a brief click around hasn't revealed any errors.
It doesn't care whether index.php is included in the url or not.
Comment #57
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedShould we have an update function that deletes the variable?
Comment #58
Crell CreditAttribution: Crell commentedNiklas: Probably yes, we should.
Comment #59
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAdding an update hook for that. I am not sure how to fix drupal.sh, so that's still open.
Comment #60
cweagans'rm drupal.sh' would be a good start. Does anyone really use it anymore?
Comment #61
katbailey CreditAttribution: katbailey commentedI agree it's probably about time drupal.sh was shown the door given that the motivation for it arose in those dark dark pre-drush days. However, it certainly deserves its own issue and sure enough, there is one for it: #1540390: Deprecate the drupal.sh script
In the meantime, so as not to let this dumb script hold things up, I've added a fix for it to the patch.
Comment #62
aspilicious CreditAttribution: aspilicious commentedI can't judge the code stuff in this patch, too much black magic happening with requests. I can verify that every instance of $_GET['s'] is removed.
Comment #63
RobLoachVERY VERY CLOSE!!!
I understand we're moving from one global ($_GET['q']) to another ($GLOBALS['script_path']). Are there plans to remove it entirely? I guess with the Session/Request stuff?
Could we use
global $script_path
at the top of the function rather than referencing $GLOBALS straight up?global $script_path;
here too? ;-)global $script_path;
plz? :-)Probably don't need the drupal.sh change now that drupal.sh is going away.
Here too plz? :-) ... I understand it's possible to reference $GLOBALS directly, but having all global references at the top of the function cleans it up a bit.
-22 days to next Drupal core point release.
Comment #64
tim.plunkettI don't know that we need to be messing with
$GLOBALS['foo']
vsglobal $foo
in this issue.I happen to agree with catch in #1515196-4: Standardize retrieval of "global" variables to be at beginning of function:
Still CNW for removing the drupal.sh hunk.
Comment #65
tstoecklerI don't see the point in breaking something intentionally in core. I fully support removing drupal.sh, but that's not for this issue. You could argue whether it is necessary to cater for drupal.sh if we know we are going to kill it, but since we already have a patch that does there is little point in removing that hunk.
Comment #66
Dave ReidAgreed. We shouldn't be discussing global $foo vs $GLOBALS['foo'] although I will say like catch and timplunkett I prefer the latter as it's more self-documenting where the variable is coming from.
Comment #67
RobLoachSounds good with me! Let's off-load those discussions to #1515196: Standardize retrieval of "global" variables to be at beginning of function and #1540390: Deprecate the drupal.sh script. I'm RTBCing this one.
[EDIT] Made note of the follow ups in the issue summary.
Comment #68
Crell CreditAttribution: Crell commentedTagging
Comment #69
chx CreditAttribution: chx commentedHold your horses. Did anyone test this on a FastCGI install? IIS? I do not see them even mentioned or considered.
Comment #70
cosmicdreams CreditAttribution: cosmicdreams commentedI'll have an opporutnity to test this on FastCGI / IIS later today.
Comment #71
cosmicdreams CreditAttribution: cosmicdreams commentedso far:
installation works fine
many pages work fine
What should I test?
Do you want me to tell my computer to run the complete test suite overnight?
Comment #72
effulgentsia CreditAttribution: effulgentsia commentedThe test should be that:
Whether success or failure, please report the version of the web server you're testing on.
Comment #73
cosmicdreams CreditAttribution: cosmicdreams commentedHere's what I can see
when I create a node and go to it. The path is localhost/index.php/node/1 . That worked
when I test content at localhost/node/1 it worked.
Recommend RTBC
I've attached my phpinfo
Comment #74
chx CreditAttribution: chx commentedOK then.
Comment #75
effulgentsia CreditAttribution: effulgentsia commentedIf that's what happened, then something is wrong. With the patch applied, there should be no code that does anything differently based on $conf['clean_url'].
Comment #76
cosmicdreams CreditAttribution: cosmicdreams commentedModified my comment. My perception was wrong. It works in both cases, without configuration change.
Comment #77
effulgentsia CreditAttribution: effulgentsia commentedThanks!
Comment #78
cosmicdreams CreditAttribution: cosmicdreams commentedOh and as for version of IIS = 7.5 express
Comment #79
RobLoachDid a bit more testing on this and if you
a2dismod rewrite
, and visit the homepage without the index.php, links don't add the /index.php/ prefix. Using index.php off the bat works fine though.Should we put some documentation in settings.php to make it default WITH the index.php, and instruct users that they can use clean_urls = TRUE to remove the index.php prefix?
Comment #80
effulgentsia CreditAttribution: effulgentsia commentedSee #43-#47. #79 is the expected behavior of this patch. IMO, this is acceptable, or at any rate, can be discussed further in a follow-up. But leaving to others to decide whether to agree and RTBC, or disagree and require resolution on that point before this goes in.
Comment #81
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@Rob Loach: As of (point 2) http://drupal.org/node/1183208#comment-5883736 that is expected behavior. The suggestion was a contrib module that redirects/
to/index.php
or find nother solution that doesn't reintroduce a variable.Everything said - crosspost.
Comment #82
RobLoachIf we have a follow up issue put together to figure out what to do with that, I'd recommend RTBC.
Comment #83
effulgentsia CreditAttribution: effulgentsia commented#1546082: Follow-up to variable_get('clean_url') removal
Comment #84
RobLoachThanks!
Comment #84.0
RobLoachUpdated issue summary.
Comment #84.1
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary.
Comment #84.2
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary.
Comment #84.3
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary.
Comment #84.4
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary.
Comment #85
effulgentsia CreditAttribution: effulgentsia commentedFYI: I updated the issue summary. If anyone wants to improve upon it, go for it.
Comment #86
catchIs there a follow-up issue for the first @todo? I'd rather all this explanation was in an issue than the code tbh.
This is also better as a follow-up issue, we can just remove that comment for now no?
This is a bit evil. While it's going to be refactored later, is it evilness that we could move to a helper function at all? I haven't read down to image styles but I'd assume something similar has to happen there too.
Are we sure there's no other side effects from removing the menu_rebuild() here?
Happy happy happy.
OK apparently the same level of tweaking isn't needed for image styles, that's encouraging then ;). However we're removing an API function and replacing it with a $GLOBALS check, could we factor that into a helper as well?
grrrr #1348162: Add update_variable_*() but NIH.
None of these are really blockers, I couldn't find a lot less to complain about although I didn't do a line by line of the entire patch, so leaving RTBC a bit longer.
Comment #87
cosmicdreams CreditAttribution: cosmicdreams commentedregarding catch's first request for a follow up issue: #1547184: Refactor Core scripts
Comment #88
effulgentsia CreditAttribution: effulgentsia commentedThanks for #87. This addresses #86.1 and #86.2, and re-adds the robots.txt and web.config changes from #54 that got accidentally dropped from subsequent patches. Other than robots.txt, these are minor comment changes only, and the robots.txt changes are trivial, so leaving as RTBC.
NIH. Opened #1547376: Allow url() or some other function to return individual components of an outbound URL and added a @todo linking to it. Adding parameters to url() might involve some debate, so not wanting to hold this issue up for it.
I'm not. But it's what the code comment claims, and tests pass.
Since you indicated that this isn't a blocker, I'd rather defer it, since it's just happening in 1 place and could get fixed with Symfony related changes. I'm hoping that before D8 is released, we remove all globals period.
Comment #89
Dries CreditAttribution: Dries commentedI'm in support of this patch. It seems like for the most part this is a big improvement; except in a couple of cases where the url() API/use got a bit more complex/ugly. In any event, I'll let catch drive this home.
Comment #90
aspilicious CreditAttribution: aspilicious commented#88: 1183208-drupal-q-88.patch queued for re-testing.
Comment #91
aspilicious CreditAttribution: aspilicious commentedretesting, just to be sure...
Comment #93
xjmIt seems to me that these @todo should simply be removed, now that the issues are filed, though the ones for
_current_path()
are clearly relevant.Comment #94
no_commit_credit CreditAttribution: no_commit_credit commentedMeanwhile, trivial docs fixes. I didn't remove the @todo since I don't know that there will be consensus on removing them.
http://drupal.org/node/1354#functions
http://drupal.org/node/1354#hookimpl (scroll down regarding
hook_update_N()
.Comment #96
xjmMisnamed the interdiff.
Comment #97
effulgentsia CreditAttribution: effulgentsia commentedPersonally, I like having @todo in the code pointing out known WTFs and incorrect documentation/comments, so that someone working on some other problem and going through that code doesn't get tripped up, and can easily find the relevant d.o. issue if there is one. But if we have a standard to do otherwise, I won't object to removing them.
Comment #98
Dries CreditAttribution: Dries commentedTalked to Larry and catch in e-mail. Catch doesn't have internet access for a couple days, and this patch is holding up some of the work Larry is doing. Therefore I just reviewed this patch again, and decided to commit it to 8.x. It is a very nice simplification that shouldn't negatively affect many people in today's world where modern web servers all support URL rewriting. Thanks to everyone who helped! :)
Comment #99
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks like this commit doesn't appear. Did you push it?
Comment #100
Dries CreditAttribution: Dries commentedI did push it. Sure you don't see it?
http://drupalcode.org/project/drupal.git/shortlog does not seem to update in real time ...
Comment #101
tim.plunkettAfter cloning, none of these have been pushed:
Comment #102
tim.plunketthttp://drupalcode.org/project/drupal.git/commit/0275068
Thanks! Updated the other issues.
Comment #103
sunThis needs a change notice.
Comment #104
xjmComment #105
catchI opened #1555598: Provide redirects for legacy ?q= URLs for the upgrade path issue.
Comment #106
webchickThis issue has been sitting for over 6 weeks without documentation, and we are well above critical thresholds atm. please get this polished off at your earliest convenience. Thanks.
Comment #107
Crell CreditAttribution: Crell commentedThe "new" approach for getting data about the current path is going to change a couple of times as we go forward with WSCCI. How should it be documented here? To current_path(), to Request->attributes->get('system_path'), or to "you're not supposed to care, that information isn't global anymore, deal"?
Comment #108
effulgentsia CreditAttribution: effulgentsia commentedI'll take a stab at this.
Comment #109
effulgentsia CreditAttribution: effulgentsia commentedI added 2 change records (they automatically show up at the bottom of the issue summary).
Comment #110.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #111
YesCT CreditAttribution: YesCT commentedas it relates to #2251061: arg(0) returns path prefix with Language Path Negotiation enabled, looking for the issue for this @todo. It says temporary... but it was April 2012.
... ah @catch helped me find it in irc.
#2237001: Remove no longer needed _current_path() fallback for early bootstrap