On test upgrades, I found out that the upgrade process pushes the PHP filter module and the PHP input format on me. Given that the primary reason to move this to a separate module is to serve security, IMHO we should not push this input format on sites (like my use case) where the PHP input format simply should not be there.

The offending update is in system_update_6009() and has a few problems:

- unconditionally removes the second input format, regardless of what it might be (although users are allowed to remove filters, rename formats, so format #2 might not be a PHP input format on a particular site) => leads to data loss
- unconditionally enables the PHP filter module
- unconditionally adds a new input format with this filter enabled

IMHO all these are wrong.

What we should do is to identify whether there was any input format which used the PHP filter, and only enable the module if there was at least one. I am not convinced we would need to overwrite any of the existing PHP input format's information (as mentioned above the code removed the old format and added a brand new one). As far as I see, the only thing we need to do is to enable the PHP filter module if required and update the input formats affected to point to the new evaluator. Although the code lives elsewhere, behavior was not changed, so caches should not be flushed, etc.

Unless there is a reason uncovered to add this as a completely new format, I think we can go with this solution. This keeps people who chose not to enable the PHP format in Drupal 5 and before without this format.

Issue where this update code was added for reference: http://drupal.org/node/46941

CommentFileSizeAuthor
stop-php-format-push.patch2.23 KBgábor hojtsy

Comments

gábor hojtsy’s picture

Assigned: Unassigned » gábor hojtsy
Priority: Normal » Critical

My suggested solution to leave the input formats using the PHP filter alone (apart from updating their reference to the filter) also solves another problem with the current update code: that it does not enable the newly added "PHP code" format for any roles, so upgrading sites where the PHP format was previously use will see that this format is not available to any roles anymore.

So the existing update code is bad for both who use the PHP filter and those who do not want to.

dries’s picture

Interesting. I agree with Gabor's explanation, and I believe his patch does the right thing. It took me a while because it is drastically simpler than the previous patch. As far as I'm concerned, this patch does the right thing, and it should be RTBC.

It would be great if Zen (author of the original PHP filter issue) could comment on this patch, but IIRC he was away from the keyboard for a while.

Zen’s picture

Status: Needs review » Reviewed & tested by the community

I agree too. This patch is a lot cleaner, simpler and as Gábor pointed out, more reliable. +1

Thanks,
-K

dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright. Committed to CVS HEAD. Thanks Gabor.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.