Pull remaining PHP handling into PHP module

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.

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

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.

About #11's last commentary, the solution is to return !$block->visibility instead of TRUE.

Taking into account that the patch in #8 is functionally complete and moves drupal_eval() out of common.inc and places it in the php module, I would recommend that it be committed. Work on creating a drupal_page_match in common.inc and removing that code from block.module could be done in a separate issue.

The attached code corresponds to #8, updated to Drupal 7's current snapshot.

João

Small comment, do we really need to inroduce a drupal_page_match() when we already have drupal_match_path?

I don't see any drupal_page_match() being introduced in the code..

It was in response to "Work on creating a drupal_page_match in common.inc and removing that code from block.module could be done in a separate issue." in #15. This will probably need some tests.

Rerolling the patch

Apparently the testing system failed to install Drupal, and the only way to restart it is to re-upload the patch.

Idem as #22

Last try..

jcnventura: As a rule of thumb, if the testbot can't install the patched site the first two times, it's probably the patch. ;)

Edit: (Assuming the testing platform is working, which you can see by find if it is passing other patches.)

@Arancaytar: it failed to install HEAD.. That has nothing to do with the patch itself.. Try downloading HEAD yourself and apply the patch. You'll see that it applies perfectly and passes all tests. You may be able to see in #20, that the patch was working perfectly until April 1st, and then the testing system began having issues to install Drupal itself. I had a patch in #222926: HTML Corrector filter escapes HTML comments that failed for the exact same reasons at the time I was trying to upload this.

I will change the patch according to nedjo's description in #29 and try again.

João

Given that your patch was the only one to fail during the period, yes, that's definitely the patch. Probably some E_ALL warning somewhere. That it "works for you" doesn't mean it should work for everyone else.

Fatal error: Cannot redeclare system_update_7021()

So it is not the patch?

Hi Damien,

Thanks for that.. Indeed it is in the patch.. Because of the similar problem in #222926: HTML Corrector filter escapes HTML comments, I was assuming it was Drupal.org being stubborn, especially because the error was "failed to install HEAD" and not "Cannot redeclare system_update_7021" as it should be...

I will move it to system_update_7999() until this becomes reviewed by the community so that I don't have to be tracking Drupal's last system_update every day..

João

"Failed to install HEAD" means that the patch broke the installer. Those are all the details you get in the summary - if the patch fails, it's either because the patch failed to apply, or because it introduces a syntax error, or because the patched Drupal is unable to install, or because a test case fails after it has installed.

However, I would also like a more detailed view on the PIFR page. Counting failures and passes isn't very helpful when you need to know which assertion failed, exactly - or what the "exception" (ie notice/warning) was. You end up having to run the unit test yourself, unless I missed a link somewhere.

"Failed to install HEAD" means that the patch broke the installer.

@Arancaytar: sometimes it means option e) that the installer is broken. At the time I was re-uploading this, as I mentioned 2 times before, I was also re-uploading a patch that started working after uploading it twice. That this patch was telling me the EXACT SAME ERROR at the EXACT SAME TIME, led me to believe naturally, that the solution would be EXACTLY THE SAME: re-upload it.

Thanks to Damien Tournoud, I was able to find out the root cause of the problem in this case.

OK,

I have changed the name of the permission as requested by nedjo in #29. The system.install update is 7022, so it may expire soon.

João

Bumping system_update to 7023 and hoping for the best..

João

Very nice clean up, I want. Don't abuse security: this solves no security issue by itself.

Oh, what a great little patch! Also, yay for permission descriptions, since "use PHP for settings" isn't descriptive enough (and I don't really see any alternatives), but the actual description of the permission is great and very clear. Hopefully with drupal_eval() moving to PHP module, we'll see contrib somewhat forced to conform to this standard as well, instead of having to set 50 different "use PHP for blah" permissions.

Committed to HEAD. Great job! :D

Gábor mentioned in #13 that it'd be great to have this in 6, and I agree it would've been, but a major API change like this is far too late for that now, I think.

Marking "needs work" because we need this to be documented at the 6.x -> 7.x upgrade page.

Hm, I should read the tags before I commit stuff. ;) "Needs tests" is also true. We can probably pick that up in another issue, though, since I don't think any of the block visibility settings are tested atm.

Well, right, it is too late for Drupal 6. So sad.

webchick, thanks for committing the patch! After 3029 commits to drupal contrib, this is the first time that code modified by me gets to be committed to Drupal core.. Yay!

Indeed, I didn't modify any test because I couldn't find one that I could modify.. In the existing php.test, the PHP filter functionality test (function testPHPFilter()) executes some PHP code introduced in a node with the PHP filter.. The execution of that filter is handled by the newly renamed php_eval() function. The only part not tested is, as you indicated, the use of PHP for visibility settings (which in core is done only by the block module).
Can we add this to a list of TBD tests for the block module and mark this issue fixed?

And Gábor, don't you dare commit this to Drupal 6... The two modules I maintain both use drupal_eval, and I would hate to have to answer to users saying that their visibility no longer works because they don't have the PHP module enabled..

As to the documentation, one needs to say that drupal_eval() is now called php_eval() and that calls to drupal_eval() should be wrapped in a call to if (module_exists('php')). And that the 'use PHP for settings' should be used.

João

Yay, congrats for the commit! :D And agreed, it looks like tests for block visibility are at #370172: refactor block visibility.

As to the documentation, one needs to say that drupal_eval() is now called php_eval() and that calls to drupal_eval() should be wrapped in a call to if (module_exists('php')). And that the 'use PHP for settings' should be used.

Right. João, can you edit the page to add that in, please? Then feel free to mark this back as fixed.

@webchick: Just to confirm, the page to be edited is http://drupal.org/node/224333?

That's the one.

Documented at http://drupal.org/node/224333#php_eval

PS: Can someone remove the 'Needs documentation' tag? I don't seem to have permissions for that...

Yes you do =)