Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Making it default to TRUE would be safest but might require quite a bit of code to be patched. Certainly, I can live with that. It also mandates that we stop using $_GET['q'] and that we run everything through arg().
Dries, usually I fulfill your requests much faster, this particular one took off only after a year you have written about it, but still, here it is. The $_GET['q'] part is not yet done but that's piece of cake.
Comment | File | Size | Author |
---|---|---|---|
#57 | 63390-no-excess-args-57.patch | 7.07 KB | pwolanin |
#55 | 63390-no-excess-args-55.patch | 7.06 KB | pwolanin |
#53 | 63390-no-excess-args-53.patch | 5.07 KB | pwolanin |
#51 | 63390-no-excess-args-51.patch | 1.36 KB | pwolanin |
#45 | menu_mask.patch | 13.42 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedNote that this is only the first step. The second will be patching menu_execute_active_handler... But first let's get this in a working shape... I guess there will be lot of things broken.
Comment #2
Dries CreditAttribution: Dries commentedThanks for working on this, chx. It's an important feature. However, this is not 100% what I had in mind. What I had in mind was a simple (optional) "type system". For example:
arg(1, 'integer');
Then, the first argument would be cast to an integer. Other types could be 'raw' and 'plain' (escaped text). The 'problem' with your version is that
arg()
can have various return values / types, which is confusing. I think it should always return the value. The comparison shouldn't take place inarg()
, IMO.The problem is that often, people trust the return value of arg(1), which they shouldn't.
Comment #3
chx CreditAttribution: chx commentedMaybe we could introduce wrappers so it's less confusing?
Now that you mention plain... yep. Raw is never needed, so the attached patch would also be enough just for security.
Comment #4
chx CreditAttribution: chx commentedneh, the second patch is not good, for example profile uses something close to raw. I will think on it.
But I still like my solution best :)
Comment #5
Wim LeersMoving to 7.x. This would be a small - yet nice feature :)
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commented@chx: can you confirm this can be closed?
Comment #7
chx CreditAttribution: chx commentedNot at all. There are ideas brewing about validating path parts in the new menu system.
Comment #8
chx CreditAttribution: chx commentedThe wish for something like this harkens back to the days when I was a Drupal freshie.
We have a new menu property: mask.
mask can be a regular expression as demonstrated -- not for the faint of heart, mind you -- in taxonomy module.
In order to save people from writing regular expressions for the most common cases, we are introducing some shorthands: '+' any character, %d means a number, %c means a word character, %t means a character that can appear in a node type -- lower and upper case letters and the underscore.
What this makes this interesting is that you can also mask the parts beyond the router path. So if you have defined foo/bar then foo/bar/%d means that foo/bar and foo/bar/123 are acceptable paths here, otherwise the path will not be found. foo/bar/%t/%d means that foo/bar/article/12 is good and so is foo/bar/article. If you want to not allow foo/bar/article, never forget that you can supply a regexp yourself. %d+ means any number of numeric arguments so foo/bar/123/1243 etc are also good.
If router path is $path then "$path/+" is what currently Drupal does. Do not let the additional slash fool you: as detailed above, the parts after the router path are always optional.
Comment #9
chx CreditAttribution: chx commentedTo clarify, node/%edit gets translated automatically to node/\d+/edit so no need to define a mask. You need a mask if your percent sign is not a positive integer or if your callback can accept arguments after the router path. The latter is automatic right now and while extermely powerful, a little validation can't hurt.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedAfter an insightful chat with chx on IRC, I'm convinced that we need the extra flexibility of this new 'mask' property.
To clarify, there is three routes hook_menu implementations might take while dealing with this new property:
The shorthand forms ease the definition of matchers for menu parts: you can use '%d', '%c' and '%t' as a menu part and the regexp will be built for you.
Obviously the code needs work (I'm not even convinced it works in its current form), but the architecture looks sane.
Oh, one more thing: I'm not convinced about the usefulness of
'%c'
. What are the use cases for this? (to be clear: this will prevent any non-word characters (such as . , - etc.) to be used).Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedCould you provide some example paths from Contrib where this will be useful? There is only one instance in this patch for core. (taxonomy/term)
Comment #12
chx CreditAttribution: chx commentedMoshe, I am fairly sure that's not the case -- any core function that takes arguments after its router path needs a mask. Contrib... for example, you have
og/subscribe/%node
and the callback function isfunction og_subscribe($node, $uid = NULL)
the mask isog/subscribe/%d/%d
. Thus the$uid
is ensured to be a number. Without the mask you can not pass in a $uid. Edit: but even with the mask the $uid is not mandatory. I admit this is not 100% intuitive.Or you have
og/approve/%node/%user/%
where the last percent sign is a token. its mask could beog/approve/%d/%d/[a-fA-F0-9]
or a simpler, less strict mask could beog/approve/%d/%d/%c
. Without the mask you can't pass in hex only numbers.Note that if you dislike copypaste then
X/X/%d/%d/%c
would do or I think the code would happily deal with even//%d/%d/%c
. Edit: this is a safety belt against typos. We might say that this feature falls in the "we do not babysit broken code" category and remove it.Comment #13
dropcube CreditAttribution: dropcube commentedsubscribing, will review later.
Comment #14
chx CreditAttribution: chx commentedcwgordon7 argues that using an array would be better than the paths. I tend to agree -- while in the router path we can skip the literal parts and most of the times we can also skip the wildcards as they are numeric anyways.
Comment #15
cwgordon7 CreditAttribution: cwgordon7 commentedUpdated patch that removes the %t, and instead lets you define constants of the form strtoupper(placeholdername) . '_MASK' (e.g. NODE_TYPE_MASK). It will then concatenate the value of that constant into the regular expression.
Also removes an accidental menu_rebuild() from index.php.
Also adds schema definition/update functions.
Also moves a lot of the heavy work of building the mask from the array into a separate function.
Also adds a test case.
Comment #16
cwgordon7 CreditAttribution: cwgordon7 commentedAfter discussion with chx in irc, we agreed that the /? was insufficient. We are now using rtrim($path, '/') before checking the path, thus maintaining the old behavior of having node/1, as well as node/1/, as well as node/1///// be valid paths. Whether this behavior should continue is up to community debate.
Note that this is at code needs review because there is no architecture needs review status, there are many known bugs that can only be fixed once the new API is fully adapted throughout all modules, which we don't want to do until we have a solid, supported architecture.
Comment #17
cwgordon7 CreditAttribution: cwgordon7 commentedUpdated patch posted, with better function names and removed trailing slash handling after realizing that path.inc does that for us.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commented#17 looks all clean, good and shiny, except for that:
First it would be cleaner to have the $suffix addition moved inside the loop (to be consistent with the $prefix). Then, I think the $mask_length is wrong, it should probably be something like:
Finally, because %d is equal to %, why not dropping it?
Comment #19
chx CreditAttribution: chx commented$suffix inside the loop won't work, it's stack like. $mask1 . $mask2 . $postfix2 . $postfix1 -- if you concat $mask1 to $postfix1 how will you stick the next ones between them. % works because it was the quickest way to code a fallback but we won't really advertise this :) . %d is the consistent mask.
Comment #20
Damien Tournoud CreditAttribution: Damien Tournoud commented@chx, ok drop my first and last comment from #18. What about the comment about
$mask_length
?Comment #21
chx CreditAttribution: chx commentedThat's one valid concern -- but it's a bit of an edge case so I commented it.
Comment #22
pwolanin CreditAttribution: pwolanin commentedjust commenting to subscribe for now. chx and I discuessed this a few weeks ago - hopefully it can add some robustness to callback handling.
Comment #23
chx CreditAttribution: chx commentedA 'rest' key is supported so that you can define any number of closing arguments.
Comment #24
dmitrig01 CreditAttribution: dmitrig01 commentedfixed an edge case, added tests for 'rest', and moved the test file to the correct location
Comment #25
chx CreditAttribution: chx commentedSpot the bug! I know where it is and time permitting I will fix it :) but the patch is still in "architecture (needs review)". (hint. it's with the rest key)
Comment #26
chx CreditAttribution: chx commentedThe problem was that the suffix needs to go after the rest, otherwise rest could replace the specified optional arguments. Adjusted the test too.
Comment #27
dmitrig01 CreditAttribution: dmitrig01 commentedBTW, the status would be "architecture (needs review)" if we had one like that. the patch will generate errors in head, because it's the architecture that needs review. if we like the architecture then the rest of the patch will be fixed.
Comment #28
maartenvg CreditAttribution: maartenvg commentedPatch no longer applies. (menu.test already exists)
Comment #29
chx CreditAttribution: chx commentedComment #30
chx CreditAttribution: chx commentedKeeping up with HEAD.
Comment #32
moshe weitzman CreditAttribution: moshe weitzman commentedI tried to review this but really don't feel qualified. I did notice that it is missing a patch to menu.api.php which described the new mask property.
Comment #33
chx CreditAttribution: chx commentedComment #35
chx CreditAttribution: chx commentedComment #37
chx CreditAttribution: chx commentedReroll against HEAD.
Comment #40
chx CreditAttribution: chx commentedPoor testbot. I left in a menu_rebuild in index.php.
Edit: phew, caught it before cron ran so the previous comment is now deleted.
Comment #41
chx CreditAttribution: chx commentedAaaaand I managed to upload the same but wrong patch. Damn.
Comment #43
chx CreditAttribution: chx commentedThe logic was a bit broken.
Comment #45
chx CreditAttribution: chx commentedI just saw dblog passing without fixing. I fixed a few modules manually but dblog passed here. Let's see what else is breaking. Note that breaking is good because it signals the patch does something useful.
Comment #47
pwolanin CreditAttribution: pwolanin commentedMinor comment - we already use "mask" for a different aspect of the menu system, so that seems like a sub-optimal term for it. Maybe 'match' or 'filter' or 'expect'?
Also, this assumes people will write the regex correctly- can we maybe have an alias for using is_numeric() or have that as a default?
Comment #48
pwolanin CreditAttribution: pwolanin commentedI guess this is too big an API change for 7.x
Comment #49
webchicksubscribe.
Comment #50
gregglessubscribing.
Comment #51
pwolanin CreditAttribution: pwolanin commentedHere is a simple patch for 7.x and 8.x that delivers a 404 if there are extra path arguments.
Comment #53
pwolanin CreditAttribution: pwolanin commentedok, well that's revealing cruft in comment, forum, and other modules. This fixes some of it.
Comment #55
pwolanin CreditAttribution: pwolanin commentedOops - put the if in the wrong place before - hopefully this should remove most of the exceptions.
Comment #57
pwolanin CreditAttribution: pwolanin commentedoops missed a {
Comment #59
soyarma CreditAttribution: soyarma commentedsubscribing
Comment #60
chx CreditAttribution: chx commentedWe are definitely not dropping this very useful feature. Drupal 8 is already going in directions I am extremely uncomfortable with and dropping another Drupal-only, Drupal-defining feature is not in the cards.
Comment #61
pwolanin CreditAttribution: pwolanin commentedIs this really a Drupal-defining feature? It seems rather a legacy of the old menu system.
Comment #62
gregglesI don't really feel like this is a "feature" of Drupal. @chx, can you provide some examples? Also, you went from working on this to being opposed to it. Is it pwolanin's approach or you've had a change of mind? Can you expand on your thinking?
Comment #63
moshe weitzman CreditAttribution: moshe weitzman commentedApparently the modern equivalent of this is at #1943846: Improve ParamConverterManager and developer experience for ParamConverters.