Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#14 | block_trim_pages47.patch | 1.11 KB | profix898 |
#11 | block_trim_pages.patch | 1.29 KB | profix898 |
#3 | eval.patch | 501 bytes | profix898 |
#2 | block-patch.txt | 648 bytes | bjaspan |
Comments
Comment #1
magico CreditAttribution: magico commentedConfirmed:
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.
Comment #2
bjaspan CreditAttribution: bjaspan commentedPatch attached.
Comment #3
profix898 CreditAttribution: profix898 commentedI 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 usedrupal_eval()
in a module.Comment #4
profix898 CreditAttribution: profix898 commentedComment #5
magico CreditAttribution: magico commented+1 for profix898 patch
Comment #6
Dries CreditAttribution: Dries commentedI 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.
Comment #7
Dries CreditAttribution: Dries commentedSorry 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().
Comment #8
Dries CreditAttribution: Dries commentedCrazy idea: why doesn't the form API trim all data?
Comment #9
chx CreditAttribution: chx commentedBecause if I touch user input then Steven guts me alive.
Comment #10
Crell CreditAttribution: Crell commentedThe 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?
Comment #11
profix898 CreditAttribution: profix898 commentedI 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.
Comment #12
neclimdulThis 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.
Comment #13
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Should be backported to 4.7.
Comment #14
profix898 CreditAttribution: profix898 commentedBackported version of the patch (for 4.7).
Comment #15
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedfixed
Comment #16
(not verified) CreditAttribution: commented