PHP Option in Visibility Settings

According to request from #136031: node selection as in blocks (visibility), I think this is a good idea to have expert mode for adsense visibility, too. Here is a fork from block.module. It should work nicely as like as that of block visibility, which I simply check its functionality with druapl_is_front_page() and all pass as expected :D

I am not very happy about this.

I don't like PHP code in the database, because it cannot be cached by accelerators, it cannot be version controlled, and hard to upgrade.

Moreover, starting with Drupal 6.x, we have a PHP module on its own so that site admins who are security aware can disable that module and have no back door, and can sleep well. This feature violates that separation.

A better approach is a hook which gets called to determine visibility, and then pluggable modules using that hook can return TRUE/FALSE to show/hide the ads. The code stays out of the database and this is extensible in many other ways.

Just for brainstorming:

I don't like PHP code in the database, because it cannot be cached by accelerators, it cannot be version controlled, and hard to upgrade.

As mentioned before this patch is just a fork of block.module, which we are using daily. So do we face the same problem with block.module? Or if not, how we solve it with block.module? If there exists some good solution, we can just simply clone it :P

...starting with Drupal 6.x, we have a PHP module on its own so...

Oh really? I am new with 6.x branch and didn't figure out this feature yet. May I have some more hints so I can test it out?

A better approach is a hook which gets called to determine visibility, and then pluggable modules...

Hmm... Seems I am over simplifying this issue... IMHO, this patch is just trying to provide something "AS IS", as like as block.module we are now using daily. It clone all permission setup so we can only allow "expert" to use PHP mode, who should understand the PROS and CONS of this model. Yes, we may work out something much complicated, but hopefully that may not be over-designed. Simple sometime means elegant :D

Hi,

Sorry to all. I was working on this feature, and I have just committed, before I even looked at this recent discussion.

I tend to agree with the fact that if the block module allows it, then there is no added risk here. Note that the PHP rule access is controlled by it's own special permission, so only the site admin and authorized users can use it.

As to hswong3i's patch. It altered a lot more than was needed (indeed making it a spitting image of the block module code). The patch as committed had less modifications and didn't suffer from the empty textarea weird case in which the visibility is always TRUE.

João

jcnventura

Make this feature dependent on the php module please. This way, those who choose to disable that module do not have the ability to store PHP in the database.

They will always have the ability to store PHP in the database, unless they disable the block module also*.

João

*I know, the block module is 'required' and not possible to disable..

For the sake of completeness, jcnventura and myself had an offline discussion. Here is the summary.

Drupal has been moving away from overly permissive use of PHP code in content. The php module is the first step. Another step is moving the PHP block settings out of the block module. See http://drupal.org/node/124158

The reasons are many and they are:

a. PHP code stays out of content (not surprises or breakage on upgrades, ...etc.)
b. PHP code gets cached by accelarators (better performance)
c. PHP code can be version controlled (better tracking of code when it gets modified)
d. The code is not a security risk (the site is safer and less likely to get exploited)

One option was to make this via a theme function like so:

function theme_adsense_visibility() {
  return TRUE;
}

And call it as

if (theme('adsense_visibility')) {
 ....
}

Then in template.php someone can do:

function phptemplate_adsense_visibility() {
  if ($_GET['q'] == ....)
     return FALSE;
  }
}

However, we have reached an agreement to leave this in the module, because for 6.x Drupal core has that for blocks.

Once Drupal 7.x (or later) moves this of block module, we will remove it from the adsense module as well, and use the same mechanism that core uses.

@kbahey and jcnventura: Thanks for this handling and I am looking for 6.x stable release. I think this is a good solution, which just clone core's PHP handling for now as well, and remove/fork it as 7.x if possible. Tomorrow we will have a better day, and so let's looking for that :D

BTW, I would still hope to promote a "complete" clone of Drupal core for adsense 5.x and 6.x, instead of a custom and trimmed version. This coming with come extra benefits:

1. It is stable. Because core code is widely used for all sites, we don't need to care about its correctness.
2. Core logic coming with some extra safety protection. If mode is now set as PHP, admin without access right will not even able to "see" the configure block. This can avoid some careless mistake and security backdoor.
3. Simpler for maintenance. If bug figure out for core logic, we just need a simple re-clone for debug.

Patch attached for revoke a complete clone of block visibility handling.

hswong3i: the code as it is now works and is being tested by +/- 100 persons.

Once this code is in Drupal 7 we will use that one, until then, keeping it in sync with Drupal block code is just annoying (i.e. if they decide to change a comma or add a blank line, should we also do it?).

Besides, the block module code has a long-standing bug which you're re-introducing with this patch... When the 'Show on only the listed pages' is set, but the page list is empty, the block module code returns TRUE on all pages instead of returning FALSE.

Since this patch is wrong and unnecessary, I don't think that the 'bonus' of being line-by-line in sync with the block module justifies the effort of applying it. Any changes that occur in that code will have to be manually merged in the module, and it's pretty trivial to apply them to the current code anyway, that nothing is gained by this.

Thanks anyway.

João