Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hi,
I just started testing using the current 7.x snapshot. Unfortunately, the install script doesn't check if json_encode() exists. Instead, the following error is produced:
Fatal error: Call to undefined function json_encode() in /var/www/localhost/htdocs/drupal-7.x-dev/includes/common.inc on line 4350
With best regards,
Timo
Comment | File | Size | Author |
---|---|---|---|
#44 | 641408-44.patch | 12.91 KB | David_Rothstein |
#40 | 641408-40.patch | 12.87 KB | David_Rothstein |
#40 | interdiff-39-40.txt | 1.68 KB | David_Rothstein |
#39 | 641408.patch | 13 KB | cha0s |
#35 | 641408.patch | 12.93 KB | cha0s |
Comments
Comment #1
GoofyX CreditAttribution: GoofyX commentedI raise the priority of this issue, because it prevents one from installing Drupal 7.0. Just tried to test 7.0-alpha1 and it failed...
Is there a solution to this?
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedWhat version of PHP are you using?
Comment #3
GoofyX CreditAttribution: GoofyX commentedI'm using 5.2.12 in my Gentoo box. I had to enable the json USE flag in order for the json methods to be compiled and enabled in PHP. Afterwards it worked fine. Can we safely assume this is a standard feature enabled in PHPs found in most hosting providers...?
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedAccording to http://www.php.net/manual/en/json.requirements.php, the JSON functions are included in PHP by default - I didn't even realize it was possible to compile PHP 5.2 without them, but apparently it is. But yeah, sounds like it's probably a pretty uncommon configuration?
I guess this Drupal 7 requirement should be documented at http://drupal.org/requirements as well as checked during http://api.drupal.org/api/function/system_requirements/7 so that a friendlier error message results?
Comment #5
GoofyX CreditAttribution: GoofyX commentedYes, Gentoo is a Linux distribution that enables the ability to selectively include or exclude major features from packages, like PHP. Since it's enabled by default and probably used by the majority of providers out there, the best thing to do is to include a friendly message (as it currently stands, no message is displayed at all, just the fatal error from the PHP engine) in case the json methods are not included in the PHP installation.
Comment #6
chx CreditAttribution: chx commentedDespite the PHP manual, there is a --disable-json switch in configure.
I think we should won't fix every issue which relates to a php not installed from php.net. I am sick and tired.
Comment #7
cha0s CreditAttribution: cha0s commentedYup, I got bit by this, too. I'm not sure how to edit the system requirements page, but I'm submitting a patch for system_requirements().
Comment #8
cha0s CreditAttribution: cha0s commentedComment #9
Dries CreditAttribution: Dries commentedGiven that a handful of people ran into this already, we should probably add this check so I committed it to CVS HEAD. If we need to add more, we can streamline the code by wrapping it in a foreach().
Comment #10
BerdirPEAR contains a nice tool called PHP_CompatInfo which can scan a directly and list all extension and constants used.
This is the output when running the whole Drupal directory:
Obviously, some of these are only used conditionally.
Comment #11
Crell CreditAttribution: Crell commentedIt may be best to just go ahead and add direct install requirements for all of the extensions we require. Even if it's stupid to run a server without some of them, it's possible so we should politely tell those people to get a real server rather than just fataling on them. If they're not cool enough to run Drupal, we should tell them so explicitly. :-)
Comment #12
jhodgdonI don't see PDO on this list. Currently, installs without PDO are not that uncommon (see #698502: Your web server does not appear to support any common database types.) and they fail during the database detection step.
Comment #13
cha0s CreditAttribution: cha0s commentedInteresting, I knew I saw that issue with not having any PDO drivers enabled somewhere. How nice it can all fit together :)
IMO, we should not be throwing an exception at #698502: Your web server does not appear to support any common database types. at all; we should have already checked that by system_requirement() time. I have submitted a patch that sort of makes the other issue a duplicate, by solving it in the requirements page... before getting to the install_settings().
So to summarize, this patch checks for all required extensions (as Dries suggested, a foreach loop works), and then if PDO is enabled, does the secondary PDO database extension check.
I'll comment over on #698502: Your web server does not appear to support any common database types. as well.
Comment #14
cha0s CreditAttribution: cha0s commentedErm, somehow I attached the old patch. Let's try again...
Comment #15
cha0s CreditAttribution: cha0s commentedSo, let's handle the edge case where someone has one or more extensions missing AND no valid PDO driver. :)
Comment #16
BerdirNote that you don't need all extensions from the used above, that's just a list of *used* extensions in the code. Especially, we have wrapper functions to use mbstring() if available and plain php functions if not. Also, some of these are only used in specific modules and already handled there, for example bcmath which is needed by openid.module.
Comment #17
cha0s CreditAttribution: cha0s commentedHmmm, but I removed bcmath from the list because I am awar of how it is used and that the OpenID module is checking for it. I didn't include it in the list.
However, you are correct -- mbstring seems like it should be optional. Updated the list.
Comment #19
cha0s CreditAttribution: cha0s commented#17: 641408.patch queued for re-testing.
Comment #20
Damien Tournoud CreditAttribution: Damien Tournoud commentedI would like to see mbstring become a requirements. After all, Drupal is nearly completely unusable without it, and our "emulated" Unicode library is way too limited.
Comment #21
xmarket CreditAttribution: xmarket commentedThanks for the effort everyone! Because you are already working on this, I just simply ask it: Are there any check for filter extension? On Gentoo you can turn it off too. According to my package.use file, I had to enable filter use flag too, to get drupal7 install work properly.
Here is the available use flags on gentoo:
Comment #22
cha0s CreditAttribution: cha0s commentedYes. My latest patch checks for filter extension.
Damien, that change is out of the scope of this issue. Other parts of core do conditional checks for mbstring, so if you want to require it then remove those checks in another issue.
Comment #23
Crell CreditAttribution: Crell commentedI'm unclear on why we're moving the database type checks up here. That part doesn't fatal out now, does it? It didn't used to.
Also, for the insanely-long error message line, can we break the array of replacement values in t() to separate lines, the way we do for database prepared statement values? It makes the code much easier to read, and is more consistent to boot.
Comment #24
jhodgdonWhy should we not check databases during the system compatibilty check? I personally think it's a great idea to make a full list of things that need to be updated at that step, so the person can go find a different host or talk to their tech support or whatever. Why wait until a different install step to figure out that they don't have a PDO-compatible database either?
Comment #25
Heine CreditAttribution: Heine commentedI would like to add to #24 that it is really silly to see a green checked "Verify requirements" and later get basic error information on another screen.
Comment #26
xmarket CreditAttribution: xmarket commented@cha0s: Great! Thank you!
Comment #27
cha0s CreditAttribution: cha0s commentedCrell, I didn't understand the complaint with the error lines. Could you clarify?
Comment #28
Dave ReidSeems reasonable to check db types in the system requirements section before we get to db selection.
Comment #29
Crell CreditAttribution: Crell commentedThis line:
Would be better as:
as it is easier to follow, easier to see what the replacements are, and matches the way nearly all DB queries are now structured.
Comment #30
jhodgdonIf the list of extensions someone could need to enable is potentially long, would it perhaps be better to format it as a UL list?
Something like this [untested]:
Comment #31
cha0s CreditAttribution: cha0s commentedSweet.
@Crell: I already do that in my own personal PHP code but I wasn't sure if I could get away with it under Drupal's standards ;)
@jhodgdon: I like the list, but I'm not sure abou the <p>, it creates a bit too much whitespace IMO. I attached a screenie of how this looks now, but if we need the p for aesthetic or cross-browser reasons, I have no problem to put it back in...
Comment #32
jhodgdonI'm fine with the UL list and no P tag.
Comment #33
David_Rothstein CreditAttribution: David_Rothstein commentedI like this patch a lot. Some comments:
$errors[]
and$failures[]
, as well asimplode('<br />', $errors)
)...Any reason not to just have two separate sections, "PHP extensions" and "Database"?
Shouldn't we include that in this patch as well? (I think just by adding
'dom'
to the list of extensions to be checked.)The formatting here doesn't seem right. If it's too long for one line, then I believe the correct adherence to coding standards would be something like this:
This should probably be "if PDO is available".
Comment #34
Crell CreditAttribution: Crell commentedChanging title, since we're talking about a lot more than just json_encode(). :-)
Comment #35
cha0s CreditAttribution: cha0s commentedExcellent suggestions. I have incorporated them into this latest patch. We also required a reroll since logic seems to have moved from install.php which was there before.
Comment #36
cha0s CreditAttribution: cha0s commentedClearer title again, IMO.
Comment #37
cha0s CreditAttribution: cha0s commentedIf I could type, that is.
Comment #38
David_Rothstein CreditAttribution: David_Rothstein commentedThis looks really good and trying it out it seems to work correctly. Very close to ready, in my opinion.
My only main comment is that for PDO, we seem to have enough information here to give them a little more detail about what is wrong - for example, we can easily distinguish between someone who does not have PDO enabled at all (and therefore should have it included in the main list of missing extensions?) vs someone who has it enabled but does not have any supported database drivers. It feels like we could massage that part a little more to make it more helpful.
Otherwise, a couple minor code style things:
These (and other places in the patch) where array elements are on their own line should always have each line end in a comma, even the last line.
This change is adding trailing white space to the patch - blank lines should be blank, so that part should be removed.
It also looks like there is some trailing whitespace further up in the patch (in the $missing_extensions list).
Comment #39
cha0s CreditAttribution: cha0s commentedOkay, I fixed the styling issues. I'm not sure about giving the user 2 separate errors for PDO and PDO extensions. Putting that patch here for review.
Comment #40
David_Rothstein CreditAttribution: David_Rothstein commentedThis looks great. I made a couple tiny changes - which can be seen in the attached interdiff file - they're tiny enough that it was easier to just make them, so I'm setting this straight to RTBC. (The only noticeable one is that I went back to the previous "Database support" wording you had before; I think that's friendlier than "Database extensions".)
Having two separate errors for PDO wasn't what I was thinking - I actually thought we'd use some logic to print the error in one place or the other, depending on which kind of PDO issue they were having. However, I like the way you did it better. My way would have involved hiding some information from people up front, which isn't good. People in this situation are likely to need all the info they can get, and even if they have the PDO extension enabled, the fact that they are seeing this error message likely means their hosting provider still doesn't really "support PDO", so the full text is appropriate. I also think it's OK to show two errors in the case of PDO not enabled at all; both errors are true. They have a missing PHP extension - so we guarantee that that list is correct and complete - but there are additional requirements due to the fact that PDO is the one that is missing, so the second error message explains that.
Comment #41
cha0s CreditAttribution: cha0s commentedLooks good! Darn commas! ;)
Comment #42
sunThis looks pretty ugly ;)
If we really want the extensions to be on separate lines, then I suggest to do:
$required_extensions = array(
...
);
upfront.
Powered by Dreditor.
Comment #43
Dries CreditAttribution: Dries commentedAgreed with sun, but otherwise looks RTBC.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedOK, I made that change in the attached patch.
Comment #45
Dries CreditAttribution: Dries commentedGreat job, and thanks for the quick re-roll. Committed to CVS HEAD. Thanks!