I have enabled unicode module and found unicode_requirements() has been triggered twice. It makes Array displayed in the list of status report. I would like to recommend changing unicode_requirements() to _unicode_requirements().

CommentFileSizeAuthor
#2 unicode_requirements_implemented_twice-2708447-11097675.patch930 bytesjimmyko
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jimmyko created an issue. See original summary.

jimmyko’s picture

jimmyko CreditAttribution: jimmyko as a volunteer commented
FileSize
unicode_requirements_implemented_twice-2708447-11097675.patch930 bytes
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Version: 7.x-dev » 8.2.x-dev
Category: Bug report » Task
Priority: Major » Normal

Or perhaps the unicode module needs to change its name. Note, the unicode module is three weeks old.

This must be opened in 8.2.x because Drupal 8 has this module.

jimmyko’s picture

jimmyko CreditAttribution: jimmyko as a volunteer commented
Version: 8.2.x-dev » 7.x-dev

@clifen

I don't think it is the module name issue as Drupal 7 does not have this core module and I can't see it will backport to Drupal 7 too. It should only affect Drupal 7, there is no reason to change it into Drupal 8 issue.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

@jimmyko Unicode_requirements exists in Drupal 8. It is unlikely you will be able to have a function name change in Drupal 7 at this stage. The unicode module has a name that conflicts with core.

jimmyko’s picture

jimmyko CreditAttribution: jimmyko as a volunteer commented

@cilefen

Ahhh.. so Drupal.org should have checking on module name when developer creates their module. So the only way is to ask the author to change the module name, right?

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue tags: +Needs committer feedback

I tagged this "Needs committer feedback" for this reason.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Assigned: Unassigned » David_Rothstein
Issue tags: -Needs committer feedback +Needs release manager review

If such a change was made only in Drupal 7 and not in Drupal 8, the contrib module would break again once it has a Drupal 8 version, so we would need to address both branches.

The core global function name is not great because it follows the pattern of an important core hook, and the module name isn't that good either since it's not exactly a unique namespace. I would definitely recommend the module be renamed since it's basically critically broken -- how does it even have a stable release if it can't be installed with D7 core?

While we could deprecate the function name in Drupal 8, we could not outright remove the old one because it would be a BC break to a public API. unicode.inc really should not exist anymore at all; #1938670: Convert unicode.inc to \Drupal\Component\Utility\Unicode was supposed to remove it. But there are still leftovers there that aren't deprecated and should be, like this function. So we should probably have an issue for that for D8.

Drupal 7 has a slightly different BC policy, outlined in https://www.drupal.org/node/1527558. Per that issue, an API change would be made for a critical issue only. While it is a very internal-ish function, a module installed on 26 sites is probably not worth the BC break. I'm not a D7 committer, though, so we should probably have one of the D7 release managers confirm. (The "Needs committer feedback" tag has been replaced for D8, so switching the tag. Also assigning directly to David so that it's more likely he will see the issue.)

Thanks @cilefen and @jimmyko.

jimmyko’s picture

jimmyko CreditAttribution: jimmyko as a volunteer commented

Thanks @xjm

I agree that the modules should be renamed due to its low popularity but the module author has no response til now. Anyway, I am glad that you have spotted this issue and help to follow. Cheers.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Title: unicode_requirements() has been implemented twice when unicode module enabled » unicode_requirements() is treated as an implementation of hook_requirements() when the Unicode module is enabled, leading to PHP warnings and an "Array" entry in the Status Report
Assigned: David_Rothstein » Unassigned
Issue tags: -Needs release manager review

The issue title is a bit confusing - I've actually used the Unicode module myself and it basically works fine :) This is just a relatively harmless PHP warning, plus a broken entry on the Status Report page.

Can't the Unicode module address this issue itself using hook_module_implements_alter()? Then no one would need to rename anything.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Title: unicode_requirements() is treated as an implementation of hook_requirements() when the Unicode module is enabled, leading to PHP warnings and an "Array" entry in the Status Report » Rename unicode_requirements(), since it is treated as an implementation of hook_requirements() when the Unicode module is enabled
Status: Active » Closed (won't fix)

I posted a patch for that at #2710151: PHP warnings and an "Array" entry in the Status Report when the Unicode module is enabled, due to conflict with the core unicode_requirements() function. Marking this issue "won't fix" for now, since I think that patch solves the problem.

jimmyko’s picture

jimmyko CreditAttribution: jimmyko as a volunteer commented

@david_rothstein I had made the same patch for me but I do think that it is just a "work around" but not a good solution. I thought that hook_module_implementation_alter is not designed to fix this kind of problem caused by inconsistent of coding standard. Of coz I know it is a harmless PHP warning, I am fine with your decision.