Per recent discussion on the development list, this patch allows modules to declare a minimum PHP version that they require. If one is not specified, the drupal core compatibility value is used (which should always evaluate to true). The main target for this patch is modules that use PHP 5-based XML handling, timezone handling, or other newer features.

To test, throw:

php = 5.5

into some module's info file, or some other version obviously higher than what you're running.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Status: Needs review » Needs work

Almost there. Reviewed and the code looks good, works as advertised, and plays nicely with my previous changes. 1 minor problem: you didn't touch update.php's update_fix_compatibilty() to enforce the php thing on upgrades. Should be a trivial re-roll, since most of the code is already in place.

Crell’s picture

Status: Needs work » Needs review
FileSize
4.32 KB

Good catch. Here's a new patch.

(I need to stop rhyming after midnight...)

Crell’s picture

FileSize
4.35 KB

See what happens when I try to rhyme? I get Notices when updating themes. This patch should work this time.

(D'Oh!)

dww’s picture

Status: Needs review » Reviewed & tested by the community

Yup, that'll do. ;)

It's pretty hard to imagine themes that require php5, so we can probably leave that out of this patch (and probably out of D6 entirely). We didn't add the explicit check for "core" to themes since D6 added .info files, so we got it implicitly already (much like .info files for modules in D5). However, perhaps in the name of usability we could extend both of these compatibility checks to themes in a follow-up issue once this lands.

Anyway, this is a good move, code is clean, tested, works as advertised, etc, etc. RTBC.

Thanks!
-Derek

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Patch looks good, and I think this is a *must* for Drupal 6, so I'll try to ensure that it gets in :) But:

1. I can imagine themes requiring a certain PHP version, eg. themes to output stuff in XML for Flex or OpenLaszlo apps.

2. It would also be nice to give some indication on what PHP version is required. You know the current error messages (both with core and PHP compatibility) tell the users: your environment is not adequate. But it does not help in *solving* the problem. Drupal should be more helpful here. It is very frustrating to be told that you are not up to standards, but it is even more so if the system does not tell you what are the standards. It should help me solve the problem, not only tell me what the problem is.

Keep up the work, I think this has a well paved way to get into core, if a bit more helpful to the user.

dww’s picture

Assigned: Crell » dww
Status: Needs work » Needs review
FileSize
10.12 KB

Well, in that case, we might as well enforce "core = 6.x" over at admin/build/themes, too. Attached patch does this, enforces php at admin/build/themes, prints a better error message in case of php version incompatibility for both themes and modules, and disables themes in update.php that aren't php compatible. Example message:

.info includes "php = 5.5.1":

"This theme requires PHP version 5.5.1 and is incompatible with PHP version 4.4.5."

.info includes "php = 5.5":

"This theme requires PHP version 5.5.* and is incompatible with PHP version 4.4.5."

Since I was touching the code anyway, I FAPI-formated some of the surrounding system_themes_form() $form array, and removed a duplicated if() to clean up the code a bit.

dww’s picture

Oh, re: error message for "core" mis-matches: I disagree. Core is the mothership. It says "you're running 6.x, that thing you're trying to install won't work here". The solution isn't to change core, but to find a different version of that module. With php it's a slightly different story. Moving php within the same version of core is much more possible and supported. But I don't think we want to imply people's core should chase the random modules they want to install. It's the other way around: people need to find modules that work for their version of core...

Also, the error message for core comes when there's nothing specified in the .info file, and there's no reasonable default. I guess we can special-case hack it that if the .info file doesn't include "core", it must be D5, but that seems wonky. Anyway, that's no reason to hold up this patch -- the core stuff went in already, and, other than using much of the same plumbing, this php version enforcing is a separate feature with different use cases and expectations. If you really want the core version tester's error messages to look different, please open a separate issue about it. ;)

Thanks,
-Derek

dww’s picture

Title: Let modules declare a minimum PHP version » Let modules and themes declare a minimum PHP version

(updating title to match the latest patch)

Gábor Hojtsy’s picture

OK, this patch looks very good. Awaiting someone to test it out to help me commit it :)

cburschka’s picture

FileSize
5.4 KB

I can't help with committing, but I can test it. I've patched my HEAD version with the file from comment #6 and it worked fine, I created a test module that announced its version as 5.5.1; here's the screenshot (never mind the inaccurate reference to PHP 6 in the description).

--

Tested these:

- Module invalidated both by a missing dependency and by PHP version: PHP version fails first; dependency does not display until after the PHP version is fixed (this makes sense though)
- Version strings that consist of less than 3 parts will have ".*" added to them. This also applies to "6.*", which will be converted to "6.*.*".
- When the module passes the version check, nothing is displayed (might be useful, but should probably be on a separate page if at all to avoid cluttering up the modules page)

Not sure on point 2 - if "6.*" is meant to be valid input, then perhaps it should not be changed to 6.*.*. If the .info line should never include asterisks, then it's irrelevant.

Quirky version strings:
- Nonsense string in .info file (not starting with a number) causes the declaration to be ignored.
- Version is correctly identified and compared numerically, even if it includes letters. "6something" will fail while "5something" will work.
- "5something.3" will however not invalidate when using 5.2.1, although "5.3" and "6something.something" will both be rejected.
- Using a version string with four parts will fail, including "5.2.2.0" with 5.2.2 (although 5.2.1.9 is fine).

Given that version strings can probably counted on to be valid (Drupal doesn't fix the mistakes of module developers), this isn't important.

I'm a bit shaky on the details of PHP's versioning convention. If they ever use strings with over 3 parts, the last point may be an issue. If they don't, well, the above applies.

Crell’s picture

PHP has well-established version number rules: http://us.php.net/manual/en/function.version-compare.php The PHP version comparison uses PHP's version_compare(), just as Drupal core's version check does. Protecting against module authors who don't know how to write version number strings is not our job.

Code looks good visually. I can't test it at work. Will do so tonight when I get home if someone doesn't beat me to it, which they are welcome to do. :-)

Gábor Hojtsy’s picture

Status: Needs review » Fixed

I reviewed the code, and it seems like there is deep testing already behind the patch, which is a critical functionality for Drupal 6 to embrace newer PHP versions, so committed.

Gábor Hojtsy’s picture

Status: Fixed » Needs work

It would be great to note this in the module/theme update guide and the drupal_parse_info_file() phpdoc would also be great to update with the new 'core' and 'php' values (as well as any more new values I missed).

Crell’s picture

Status: Needs work » Fixed

I've updated the module update page accordingly. I'll have to get to the rest of it later. :-)

dww’s picture

FYI:
http://drupal.org/node/137062 (page about writing .info files for themes)
http://drupal.org/node/137069 (documentation issue about cleaning up/re-org'ing .info-related docs)

Anonymous’s picture

Status: Fixed » Closed (fixed)
moshe weitzman’s picture

any reason we didn't use hook_requirements for this? all i can think of is that we want a filter for php version on drupal.org download pages.

Gábor Hojtsy’s picture

Having this declaratively in .info files helps module authors, they don't need to repeat the same code in all modules requiring a PHP version. And it helps automated tools, such as project module, to know what PHP version the module is compatible with.