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
| Comment | File | Size | Author |
|---|---|---|---|
| stop-php-format-push.patch | 2.23 KB | gábor hojtsy |
Comments
Comment #1
gábor hojtsyMy 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.
Comment #2
dries commentedInteresting. 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.
Comment #3
Zen commentedI agree too. This patch is a lot cleaner, simpler and as Gábor pointed out, more reliable. +1
Thanks,
-K
Comment #4
dries commentedAlright. Committed to CVS HEAD. Thanks Gabor.
Comment #5
(not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.