If PHP filter is empty, module should return TRUE and not execute code

By default you need set return TRUE or it defaults to FALSE

This is ABSOLUTELY critical!
The module should definitely behave like the title describes and a new version should be released. I wasted 4 hours searching for the reason why many of my blocks were not showing up whatever I did.... :(

The reason was the empty php block visibility field...

Use the latest release.

Hello hass,

thank you very much for your quick reply.

Well I did but it didn't work for already existing blocks. Furthermore I can't see any related code fixing this. Or have you just created a new release?
This issue also doesn't contain a commit message. I'm sorry if the mistake is on my side.

PS: I'll retry the dev version and provide feedback to make sure I'm not wrong.

No it definitely doesn't work. Patch attached.

http://cgit.drupalcode.org/php/commit/?id=0018afb

I've tried this and I think it still needs work.

The default in D7 is to return TRUE unless the PHP filter has code which returns FALSE for access allowed.

This means that all the following show the block on all pages (inverted commas excluded):

• ''
• ' '
• 'Some text'

In D8, if you do not have <?php Return TRUE; ?> as the field, then the block does not display. For upgrades from D7, this means that every PHP filter has to be checked. (The Security Review module will tell you which nodes have PHP filters, but it didn't work for me in D8, and I'm not certain that it tells you about blocks. It would be useful to have a report of which content has the filter without Security Review, wouldn't it?.)

The patch above works as long as the PHP filter is empty. [Edit: by empty I mean unset.] But if it's had anything in it [Edit: I mean set, and it could even be set to ''], then this:

web/core/lib/Drupal/Core/Condition/ConditionPluginBase.php:

 public function setConfiguration(array $configuration) {
    $this->configuration = $configuration + $this->defaultConfiguration();
    return $this;
  } 

causes the problem because when the block is instantiated because

$configuration = {array} [4]
 id = "php"
 negate = false
 php = ""
 context_mapping = {array} [0]

And bam! that value for the php field is used instead of the default from the patch.

I've only thought of one possible solution, and it's not elegant... Have two fields instead of one for the php field. One which contains a raw value (i.e. the current php field, but renamed - say php_raw), and one which is dynamically created if php_raw contains the string '<?php' - the new field would be called 'php' so that the existing code all continues to work.

Maybe there are other ideas? Or do all upgrades from D6 or D7 not work properly?

[Edited for clarity.]

Well the current approach is not perfect, but it works as it should / designed. I thought about rebuilding the old radio boxes on top of the code area. These has enabled/disabled the PHP filter in past. At least it makes the configuration easier, but it would also not solve your issue.

But what is clearly wrong and must break is random text, double/single quotes. This is not PHP code. You had some luck that is worked in past. And as always with wonkey implementation they may break in future.

I'm open for usability enhancements.

I sympathise with random text, but I think it's a bug if it's empty - i.e. '' or ' ' (or even any number of spaces, tabs, returns - blank text) and that causes the block not to appear, since it changes the semantics from D7. Especially if D6 upgrades (as is the case when I found this) cause the problem.

This is not a support request. This is a major logic bug. Blocks disappear after this module is installed.

This is a major issue indeed.

As of now, the Php Condition plugin forces every newly configured block to hide by default, I experienced this after adding blocks created via the “Custom block library” UI. They would not show up because of this bug.

And any block is also hidden by just clicking “Configure“ in the “Block layout” page and then “Save block” without even changing anything in their configuration.

The commit mentioned in #15 is a step in the right direction but it's not enough as explained by Jeff Veit in #16.

The patch in #13 would work but it's incomplete because the javascript side of the UI expects the string '<?php return TRUE; ?>' to set the 'Not restricted' label.

Given the analysis from #16 I'd say that the proper way to fix this could be to override setConfiguration, like I am proposing in the attached patch.

Mmh, the latest version of the module 8.x-1.0-beta2 with the commit from #13 does seem to work indeed.

The problem here was that composer require drupal/php was by default pulling in 8.x-1.0-beta1 instead, I am not sure why, and that version is severely bugged.

So the patch I proposed in #21 is not strictly necessary if you create the blocks after installing the latest version because the default configuration will be set to <?php return TRUE; ?> and everything will just work. Can someone else confirm this?

I am hiding my patch and showing again the previous one which can help if some blocks were created with an old version of the module.

A long term solution to avoid this problem with the form content could be to validate it when saving it, making sure that the code is inside the php tags, this way the user could never set an empty value.