If $library['version callback'] uses a pass by reference parameter (and does not have $library['version arguments'][0]> set, then a warning is thrown in a PHP 7.0 environment. Issue can cause referenced library not to load correctly. This was specifically seen with the Navbar module which used the function _navbar_libraries_get_version(&$library, $options = array()).

Example of the error:

Warning: Parameter 1 to _navbar_libraries_get_version() expected to be a reference, value given in libraries_detect() (line 549 of libraries.module).

That particular line uses call_user_func() which only passes parameters by value and never by reference. It needs to be changed to call_user_func_array(), which is actually used as the other version callback function for a slightly different branch scenario.

Bug was seen originally in 7.x-2.3. Verified bug still exists in 7.x-2.x-dev.

CommentFileSizeAuthor
#11 libraries-version-callback-reference-parameter-fix-2779591-11.patch683 bytesmark_fullmer
#10 libraries-version-callback-reference-parameter-fix-2779591-10.patch759 bytesmark_fullmer
#8 libraries-version-callback-reference-parameter-fix-2779591-8.patch831 bytesmark_fullmer
#3 libraries-version-callback-reference-parameter-fix-2779591-3.patch683 bytescglauren
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cglauren created an issue. See original summary.

cglauren’s picture

cglauren CreditAttribution: cglauren commented
Version: 7.x-2.2 » 7.x-2.3
Issue summary: View changes
cglauren’s picture

cglauren CreditAttribution: cglauren commented
FileSize
libraries-version-callback-reference-parameter-fix-2779591-3.patch683 bytes
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Category: Bug report » Feature request
Status: Active » Needs review

This is definitely a feature request. Version callbacks currently have one - and only one - purpose: Return a version. They are not allowed to modify the library at the moment. I.e. the fix would be removing the ampersand in the function declaration of Navbar module. Can you elaborate on the use-case for this?

On the other hand, this looks very harmless. I see no reason not to commit this. Sending for a test run for now.

This might also be a nice way to fix #2167249: Make version/variant callbacks provide error messages themselves for better DX/UX. without breaking BC, if we add some docs that say that version callbacks should populate $library['error message'] or something like that.

Ace Cooper’s picture

Ace Cooper CreditAttribution: Ace Cooper as a volunteer commented

@cglauren Thank you! This patch just saved me a lot of grey hair.
I was checking out the QuickEdit module, so I installed both JS dependecies: backbone/backbone-min.js and underscore/underscore-min.js into the libraries folder. But for the life of me I couldn't get their versions to show up on the Status page, instead receiving the status:

The version of the Backbone library could not be detected.
The version of the Underscore library could not be detected.

I repeatedly checked if backbone/underscore versions mismatched the prerequisites.
Then I though that maybe my libraries or jquery_update version could be wrong.
I also noticed a couple of these errors in my watchdog:

Warning: Parameter 1 to _quickedit_libraries_get_version() expected to be a reference, value given in libraries_detect() (line 556 of /sites/all/modules/libraries/libraries.module)

A short search landed me here and luckily this patch #3 fixed the problem.
Now both Backbone 1.3.3 and Underscore 1.8.3 are detected correctly.

robertgarrigos’s picture

robertgarrigos CreditAttribution: robertgarrigos commented

This patch fixed it for me also.

maximpodorov’s picture

maximpodorov CreditAttribution: maximpodorov commented
Version: 7.x-2.3 » 7.x-2.x-dev
Status: Needs review » Reviewed & tested by the community

The patch works perfectly.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
CreditAttribution: mark_fullmer as a volunteer and at University of Texas at Austin commented
Status: Reviewed & tested by the community » Needs review
FileSize
libraries-version-callback-reference-parameter-fix-2779591-8.patch831 bytes

I can confirm the patch works on PHP 7.1.1 as well. The patch in #3 however, does not apply against the latest version of 7.x-2.x-dev. Re-roll attached.

Status: Needs review » Needs work

The last submitted patch, 8: libraries-version-callback-reference-parameter-fix-2779591-8.patch, failed testing.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
CreditAttribution: mark_fullmer as a volunteer and at University of Texas at Austin commented
FileSize
libraries-version-callback-reference-parameter-fix-2779591-10.patch759 bytes

Patch re-rolled with proper diff directories.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
CreditAttribution: mark_fullmer as a volunteer and at University of Texas at Austin commented
Status: Needs work » Needs review
FileSize
libraries-version-callback-reference-parameter-fix-2779591-11.patch683 bytes

Third time's the charm.

svenryen’s picture

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

Reviewed and tested. It would be nice to get this added to the next release :)

szeidler’s picture

szeidler CreditAttribution: szeidler at Ramsalt Lab commented

I haven't verified why the navbar module uses this unconventional approach, but I can confirm, that the patch #11 is fixing the issue in my PHP 7.1 environment.

efpapado’s picture

efpapado CreditAttribution: efpapado at Ramsalt Lab commented

+1 RTBC, please commit.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented

Tested with PHP 7.1.9, patch works as advertised.
RTBC++

caminadaf’s picture

caminadaf CreditAttribution: caminadaf at CI&T for Pfizer, Inc. commented

+1 RTBC tested with PHP 7.1.8

dscl’s picture

dscl CreditAttribution: dscl at DevBrains commented
Assigned: cglauren » Unassigned

+1 RTBC
Tested on 7.1.13.

Unassigned it considering the work is done and nobody is actually working on it.
It would be great to see it committed! :)

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented

+1 RTBC. Unfortunately the last commit to the 7.x branch was nearly 1.5 years ago =/

Hopefully we can get the attention of a maintainer to get this committed.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Hello, can someone please take over maintainership of this project and publish a release with this fix?
Here is how:

  1. Contact all the current maintainers asking for maintainership
  2. If no answer, open an issue in the project ownership queue.
  3. Link the project ownership issue to this issue and mention that you've already contacted the maintainers
  4. Also mention that this project has not gotten a release in over 18 months.

Then @dddave will evaluate and likely grant you maintainership, then you will be able to commit this patch and tag and publish a release.

https://www.drupal.org/project/projectownership

For clarification, review this page https://www.drupal.org/project/projectownership

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
CreditAttribution: osopolar commented
Issue tags: +PHP 7.0 (duplicate)

#11 works for me too.

Kris77’s picture

Kris77 CreditAttribution: Kris77 commented

Patch #11 work for me too with Drupal 7.59 and PHP 7.1

Thanks to @mark_fullmer.

nikolabintev’s picture

nikolabintev CreditAttribution: nikolabintev commented

#11 works for me with D7.59 and PHP 7.1

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

OK thanks I am now a maintainer of libraries please ping me to remind me to commit this and publish a release if I have not done so by Oct 1st

Also, If there is some other important patch we need for this coming release let me know asap. If you want to help please create a release plan issue and link to this and other suggested patches for the release.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Reviewed & tested by the community » Fixed

Libraries 7.x-2.4 has been tagged and released.

To thank me, I ask each of you to become a maintainer of at least one module on Drupal.org (if not already a maintainer).

Here is how to become a maintainer:

  1. Contact all the current maintainers asking for maintainership
  2. If no answer, open an issue in the project ownership queue.
  3. Link the project ownership issue to this issue and mention that you've already contacted the maintainers
  4. Also mention that project X has not gotten a release in over X number of months.

Then @dddave will evaluate and likely grant you maintainership, then you will be able to commit this patch and tag and publish a release.

https://www.drupal.org/project/projectownership

For clarification, review this page https://www.drupal.org/project/projectownership

Thanks everyone!

sjerdo’s picture

sjerdo
Primary language Dutch
Location Haarlem
CreditAttribution: sjerdo at Synetic commented

@joseph.olstad
Seems like this issue is not included in CHANGELOG.txt.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

it's in the release notes. see here: https://www.drupal.org/project/libraries/releases/7.x-2.4

CHANGELOG.txt is a throwback to the days of CVS. If you want to become a co-maintainer feel free to do so and update it.

Now with git, all the history is available in all the clones.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.