Posted by neclimdul on August 12, 2009 at 8:54pm
7 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Provide a hook_drupal_goto_alter to allow alter drupal_goto urls and data prior to the redirect. Pretty straight forward and helps #1577: Mapping privilege separation on non-SSL/SSL.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal_goto_alter.patch | 648 bytes | Idle | Failed: Failed to install HEAD. | View details |
Comments
#1
The last submitted patch failed testing.
#2
Real patch with tests.
#3
Btw, the tests area a little ugly but they do what they're suppose to. I'm open to anyone that's got more elegant solutions.
#4
Patch seems to be stuck now but passed previously. http://testing.drupal.org/pifr/file/1/11240
#5
Just bumping testing so we can get this in. Same patch, should be a solid commit if someone wants to review.
#6
+function common_test_drupal_goto_land() {The function name is odd. goto l() and fail? Go to land? Which land? It makes sense after staring at it, but phpdoc on these functions would've helped, maybe calling it 'landing' instead of 'land'? Very minor, otherwise looks ready.
#7
Merged into #1577: Mapping privilege separation on non-SSL/SSL.
#8
chx asked me to look at this. The patch is missing PHPdoc, hook documentation, and has some coding standards violations so I didn't look too closely at it since it's clearly not ready for committer review.
This is a bit unfortunate, that these function signatures don't match:
function drupal_goto($path = '', $query = NULL, $fragment = NULL, $http_response_code = 302)...
+ drupal_alter('drupal_goto', $args);
But chx pointed out that this is because those are the params drupal_alter() takes.
What would help a lot are some use cases for why this hook is useful, since it kinda seems like a weird hack, and introduced Yet Another Way To Redirect Form Submissions. Are there use cases other than SSL? And even for SSL, how does this stop me from manually typing in http://example.com instead of https://example.com?
#9
For SSL it does not stop you, actually the redirect happens from HTTPS to HTTP because the point in the SSL patch is to provide a mechanism where most requests can be HTTP while only those that badly need security require HTTPS. Anyways, the SSL patch has all the things you want and can be separated out again to here. Also, motivations besides the HTTPS dance is response code, the API example is where misery module changes the response code to 500. In general, providing more alters when there is so little performance penalty is just good. That's what makes Drupal powerful.
#10
This was merged into the SSL issue where it was fixed up a bit. Not sure if we want to separate out or what #8 and #9 are saying.
#11
I've mostly left this issue open because of that uncertainty.
I haven't done any of the updates done to this hook in #1577: Mapping privilege separation on non-SSL/SSL as, for the the time being it, would be somewhat of a waste. There are many useful tests and integrations in the other issue that would not fit in this issue and it would take time to unravel them.
Really, I'd be happy closing this out and sticking with the other issue if we're pretty sure this is not going to get nixed out.
#12
committed in #1577 so closing this.
#13
what are the odds for a 6.x backport getting in?