Using the "Show if the following PHP code returns TRUE" block visibility option, the block will unexpectedly be shown if the textarea contains whitespace outside the <?php ?> block. This is because

drupal_eval(" <?php return FALSE ?>")

evaluates to " " which is TRUE. This is what block.module:627 in Drupal 4.7.0 does.

One easy way to fix the problem is to change line 627 to read

$page_match = drupal_eval(trim($block->pages));

Thanks,

Barry

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

magico’s picture

Version: 4.7.0 » x.y.z
Priority: Minor » Normal

Confirmed:
1. go to ?q=admin/build/block
2. configure one block
3. choose the option "Show if the following PHP code returns TRUE"
4. use the PHP code of this issue (with spaces)
5. the bug shows up.

bjaspan’s picture

Status: Active » Needs review
FileSize
648 bytes

Patch attached.

profix898’s picture

Component: block.module » base system
FileSize
501 bytes

I can confirm this bug and the patch solves it.
But I think it should be globally fixed in drupal_eval() function. Because this also happens when you put PHP code into a node or use drupal_eval() in a module.

profix898’s picture

Title: Block PHP visibility settings depend on whitespace » results of drupal_eval() depend on whitespace
magico’s picture

Status: Needs review » Reviewed & tested by the community

+1 for profix898 patch

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I don't think it makes sense to fix this in drupal_eval(). This is only relevant if we want to case the output of drupal_eval() to a boolean. For node and block bodies (i.e. PHP filter) this isn't the case. It is only the case for the block visibility.

Also, rather than having to do a x trim()s for each page view, maybe it is better to trim() $block->pages in the validate function of the 'save block' form handler? That reduces the processing cost.

Dries’s picture

Sorry for all the typos. What I'm saying is:

1. the trim() is only useful for a subset of all calls to drupal_eval(). No need to trim() inside drupal_eval() itself.

2. we can eliminate the trim() if we clean up the data before it goes into the database. It is best to clean up the data 'on input' rather than 'on output'. It might save millions of calls to trim().

Dries’s picture

Crazy idea: why doesn't the form API trim all data?

chx’s picture

Because if I touch user input then Steven guts me alive.

Crell’s picture

The input filters we use at work that I wrote auto-trim all strings, although they have a flag to not do so. In the past year, I have not once had to use that flag. I really can't think of any use cases where stripping leading and trailing whitespace from user input is undesirable. Other futzing, sure, but trim() seems a natural thing to do to anything.

If we really really need it, I suppose a "don't trim" flag may make sense. I'd be fine with just trim()ing everything, though. Or perhaps for this use case just making trim() implicit in the PHP-code filter?

profix898’s picture

Status: Needs work » Needs review
FileSize
1.29 KB

Crazy idea: why doesn't the form API trim all data?

I dont think its necessary to trim all input in FAPI, in most cases thats far too much! This is a simple issue, which shouldnt call FAPI validation into question.

Here is a trivial patch which simply trims the string before it gets saved to database.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

This works as expected. I agree with the solution as well. It addresses Dries concern and fixes it before going to the database.

Also, I agree that we should handle this as a special case rather than pulling it into fapi. It really is a special case as we expect the php to return a bool rather than returning or printing text like most php input. As such, addressing it in this manner in the block module seems fitting.

This is very straight forward and works so I'm going to RTBC it.

Dries’s picture

Version: x.y.z » 4.7.4

Committed to CVS HEAD. Should be backported to 4.7.

profix898’s picture

FileSize
1.11 KB

Backported version of the patch (for 4.7).

killes@www.drop.org’s picture

Status: Reviewed & tested by the community » Fixed

fixed

Anonymous’s picture

Status: Fixed » Closed (fixed)