Pull PHP page matching into PHP module; make page matching reusable

A grep of the /modules directory in contrib suggests the relevant code from core is repeated in various ways in 14 contrib modules so far. So this simple patch would save hundreds of lines of surplus code in contrib as well as improving consistency.

Nedjo, this is a great idea.

However, do you think we could take it on step farther and also encapsulate the proper form that goes along with this function too? Since that function isn't as useful without the form.

This will make things easier. +1

But this may open some security holes in the future.

Module-X uses this function to make decision about visibility.
Site admin let User-A to configure this module.
User-A can evaluate PHP code if Module-X has a broken logic to set the visibility value. If User-A can set it above 2, he/she can do anything.

This may not be a common scenario but IMO we should at least explicitly warn module writers about it.

If this gets in, http://drupal.org/node/106559 needs to be revisited. If that gets in, this needs to be revisited.

@m3avrck, good point. Here's an updated patch that pulls the form fragment for page matching into a separate reusable function.

I've also pulled the relevant permission into a new more generalized one, 'use PHP for settings' (not entirely sure that's a self-explanatory name for the permission, any better suggestions?), that can be referenced by all modules offering PHP settings. This in itself is an important improvement. PHP access should always be restricted. Existing modules out there implementing page matching presumably either introduce an additional permission or leave this open to e.g. everyone with 'administer site configuration' permission.

@erdemkose, the new form handling and permission should help prevent such security holes.

@chx, thanks for the reference, these two patches seem to be complimentary. I'll try to keep an eye on http://drupal.org/node/106559 and update this patch accordingly.

(I haven't been able to test this patch yet as I can't get a new HEAD install going.)

This now belongs in our new PHP module.

Additions/changes to the previous patch:

* move PHP input to php.module
* move drupal_eval() to php.module
* add an update to change existing 'use PHP for block visibility' permission to new 'use PHP for settings'

In sum: this patch accomplishes two aims: (1) complete the move to restrict potentially dangerous PHP input to the PHP module; (2) make page matching reusable to prevent the current proliferation of code copy and pasting.

I marked http://drupal.org/node/134779 a duplicate.

Refreshing the patch.

Nice patch.

Tested on an upgraded database (not clean, upgraded all along from 3.x). And on a new, clean site.
Both work well. The upgrade code does what it needs to do: move the permissions to (ab)use PHP, if set, to 6.x php module.
The php module now handles all the functionality: when php.module is disabled I could not use 'php block visibility' to root the entire server :).

However, two minor things. IMO not enough to keep this patch from going in, but still enough to have a look at, or else document: As soon as I disabled PHP module, all blocks with PHP-for-block-visibility no longer showed up. This is probably a 'feature' but some may classify it as a bug.
The other is that we will have to document more properly the fact taht the function 'drupal_eval' is now only callable if php.module is enabled. Not many contribs use it, but this may (will) break some sites that don't check for an existing/enabled php.module.

The last patch looks like it is missing a chunk ...

Purely visual review for #6:

• I think the php.install updates should probably be moved to system.install.
• Should drupal_eval now be php_eval?
• I believe that constant declarations now require phpdoc style comments for API module compatibility.
• Permission: Use PHP during configuration perhaps?
• I don't think the amendment to the description in the .info file is necessary. Makes sufficient sense as it was IMO.
• Pre-existing bug that can probably be fixed here:
  +      $page_match = $block->pages ? drupal_page_match($block->visibility, $block->pages) : TRUE;
  The above line will return TRUE even if $block->visibility is set to PAGE_MATCH_ONLY_LISTED and $block->pages is empty.

I will test against HEAD once the patch in #8 has been re-rolled correctly. I also think that Bèr's point (on how to handle existing PHP configurations when the PHP module is disabled) is a good one.

Thanks Nedjo.
-K

Thanks for the quality reviews and apologies I haven't found time to follow up with a new version of the patch. I intend to, but obviously not in time for code freeze.

Wow, this would have been great, if done for D6. I wish this is solved early in D7, or given that it is a security related improvement, maybe even in D6.