_unicode_check() in includes/unicode.inc is not returning the description string and severity constant if mbstring is installed. This creates a PHP notice when unicode_requirements() tries to get those values from the array _unicode_check() returns.

This patch adds a description and severity constant to the return value to be more consistent with the other possible return values of _unicode_check() and eliminate the PHP notice.

CommentFileSizeAuthor
#14 unicode-notices_0.patch5.61 KBwebchick
#6 unicode-notices.patch576 byteswebchick
#5 unicode_check.diff_1.txt5.59 KBdavemicc
#3 unicode_check.diff_0.txt5.7 KBdavemicc
unicode_check.diff.txt704 bytesdavemicc

Comments

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

Applies, works, necessary.

Steven’s picture

Steven commented
Status: Reviewed & tested by the community » Needs review

The lack of a description was intentional. The status report page should avoid displaying additional information when nothing is wrong.

I'd prefer a patch that simply gets rid of the PHP notice instead.

davemicc’s picture

davemicc commented
StatusFileSize
new unicode_check.diff_0.txt5.7 KB

This patch returns a blank description string.

There is another problem here. The "severity" constants REQUIREMENT_OK, REQUIREMENT_WARNING, REQUIREMENT_ERROR, and REQUIREMENT_INFO are not defined when _unicode_check() is called from unicode_check() (only when called from unicode_requirements(), they are defined at the top of install.inc). This produces an undefined constant notice when mbstring is not correctly installed or all the time with this patch.

_unicode_check() does not need to return the severity constant at all because it already returns another constant that describes the status of unicode support. This patch removes the third array item in all the possible return values of _unicode_check() and decides which requirement constant to use with a switch statement in unicode_requirements().

Steven’s picture

Steven commented
Status: Needs review » Needs work

-1 messy code. The switch statement can be written more compact by using an associative array map.

davemicc’s picture

davemicc commented
Status: Needs work » Needs review
StatusFileSize
new unicode_check.diff_1.txt5.59 KB

Uses associative array map.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Inconsistent return values from _unicode_check() creates PHP notice » E_ALL compliance: Inconsistent return values from _unicode_check()
Version: x.y.z » 5.x-dev
StatusFileSize
new unicode-notices.patch576 bytes

Am I crazy, or is this the only thing that needs to get done?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

Nevermind. ;) I should read issues. Patch in #5 no longer applies though. Re-rolling...

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Reviewed & tested by the community

Geez!! Once again, I'm an idiot. Anyway, patch in #5 applies fine and gets rid of the notices. RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: E_ALL compliance: Inconsistent return values from _unicode_check() » E_ALL compliance: _unicode_check() inconsistent return values
drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Fixed » Needs review

Neil, I think you committed the wrong patch (#6) while webchick recommended #5. Should this be backprted?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Ack.. yes, patch in #6 should be removed in favour of #5. Sorry about that. :( Now there's still a notice about an undefined constant REQUIREMENT_OK.

As for back-porting, it doesn't look like it. These fixes were all around stuff added by the installer. I just removed the ^ E_NOTICE from the line in common.inc in 4.7 checkout, and don't see any errors about unicode.inc, so I think 4.7 is ok (though there are a bunch of others ;)).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Reviewed & tested by the community

Marking RTBC so Neil looks at this again. Patch in #5 of course no longer applies, but will after a patch -R of #6.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
StatusFileSize
new unicode-notices_0.patch5.61 KB

And just for good measure, here's a patch that does this.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)