Secure argument passing

Thanks 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 in arg(), IMO.

The problem is that often, people trust the return value of arg(1), which they shouldn't.

Moving to 7.x. This would be a small - yet nice feature :)

@chx: can you confirm this can be closed?

After 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:

• Don't bother, if you only have numeric placeholders
• Use the shorthand forms, if you only have simple needs (mixing numeric and text arguments)
• Go the full regexp route, if you have complex urls or if you don't like simple solutions

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).

Could you provide some example paths from Contrib where this will be useful? There is only one instance in this patch for core. (taxonomy/term)

subscribing, will review later.

Updated 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.

After 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.

Updated patch posted, with better function names and removed trailing slash handling after realizing that path.inc does that for us.

#17 looks all clean, good and shiny, except for that:

  $mask_length = max(array_keys($item['mask']));
  for ($i = 0; $i <= $mask_length; $i++) {
    // build $mask_part
    $mask .= $prefix . $mask_part;
  }
  $mask = '#^'. $mask . $suffix .'$#';

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:

  $mask_length = max(array_keys($item['mask'])),max(array_keys($item['_parts']));

Finally, because %d is equal to %, why not dropping it?

@chx, ok drop my first and last comment from #18. What about the comment about $mask_length?

just commenting to subscribe for now. chx and I discuessed this a few weeks ago - hopefully it can add some robustness to callback handling.

fixed an edge case, added tests for 'rest', and moved the test file to the correct location

BTW, 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.

Patch no longer applies. (menu.test already exists)

patching file includes/menu.inc
Hunk #5 succeeded at 2408 (offset 16 lines).
Hunk #6 succeeded at 2436 (offset 16 lines).
The next patch would create the file modules/simpletest/tests/menu.test,
which already exists!  Assume -R? [n] 
Apply anyway? [n] y
patching file modules/simpletest/tests/menu.test
Patch attempted to create file modules/simpletest/tests/menu.test, which already exists.
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file modules/simpletest/tests/menu.test.rej
patching file modules/system/system.install
Hunk #1 succeeded at 876 (offset -4 lines).
Hunk #2 succeeded at 3056 (offset 7 lines).
patching file modules/taxonomy/taxonomy.module
patching file modules/taxonomy/taxonomy.pages.inc

I 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.

Minor 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?

I guess this is too big an API change for 7.x

subscribe.

subscribing.

Here is a simple patch for 7.x and 8.x that delivers a 404 if there are extra path arguments.

ok, well that's revealing cruft in comment, forum, and other modules. This fixes some of it.

Oops - put the if in the wrong place before - hopefully this should remove most of the exceptions.

oops missed a {

subscribing

Is this really a Drupal-defining feature? It seems rather a legacy of the old menu system.

I 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?

Apparently the modern equivalent of this is at #1943846: Improve ParamConverterManager and developer experience for ParamConverters.