libraries_get_path() should not anymore return 'sites/all/$library' as a best effort. We're better than that now. As sun correctly pointed out in #958162: Allow groups of callbacks to be applied to libraries this constitutes an API change, and, since we don't have a release for D7 yet, will break a lot of sites out there. So this should/can only be done after we tag an alpha release in Drupal 7. Marking postponed on #881362: Ready for another tagged release? because of that.

CommentFileSizeAuthor
#10 libraries_get_patch_false_2.patch1.91 KBtstoeckler
#6 libraries_get_patch_false.patch815 byteststoeckler
#2 961476-libraries_get_path.patch977 byteststoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Remove 'default' return value from libraries_get_path » Make libraries_get_path() return FALSE by default
Priority: Normal » Major
Status: Postponed » Active
Issue tags: +API change

Ideally, we'd hear from heavy users like acquia or devseed about their preference regarding this API change. If there will be no feedback, then I'd say to simply do the API change, without any alpha release upfront. Technically, we're still in "code thaw"...heheh ;)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Title: Make libraries_get_path() return FALSE by default » Remove 'default' return value from libraries_get_path
Priority: Major » Normal
Status: Active » Postponed
Issue tags: -API change
FileSize
961476-libraries_get_path.patch977 bytes

A simple patch.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Title: Remove 'default' return value from libraries_get_path » Make libraries_get_path() return FALSE by default
Priority: Normal » Major
Status: Postponed » Active
Issue tags: +API change

Oh sorry, crosspost.
Sure.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Active » Needs review

Now that we have the new branch, this should be less problematic. Marking needs review, to see if the bot picks it up. It should still apply, but you can never be sure...

Status: Needs review » Needs work

The last submitted patch, 961476-libraries_get_path.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
libraries_get_patch_false.patch815 bytes

Here's a reroll.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Doesn't seem to be covered by tests yet though?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

No, this is not properly tested yet.
I don't really know how that would work, though, because we would have to put something in sites/all/libraries for testing libraries_get_path() to be at all meaningful.

Another thing, is that libraries_detect() currently does:

if (!isset($library['library path'])) {
  $library['library path'] = libraries_get_path($library['machine name']);
}
if (!file_exists($library['library path'] || !file_exists($library['library path'])) {
  $library['error'] = 'not found';
  ...
}
...

That means that for each library which doesn't have library path set we still run a useless file_exists() on the path returned by libraries_get_path(). Now it could be considered micro-optimization (which is why I didn't include it in this patch), but we could also do for instance:

if (isset($library['library path']) && !file_exists($library['library path'])) {
  $library['error'] = 'not found';
  ...
}
elseif (!isset($library['library path'])) {
  $library['library path'] = libraries_get_path($library['machine name']);
  if (!$library['library path']) {
    $library['error'] = 'not found';
    ...
  }
}
...

I don't really like that code flow though. There might be a nicer way to do that, though, which is why I'm bringing it up.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Reviewed & tested by the community » Needs review

I don't really know how that would work, though, because we would have to put something in sites/all/libraries for testing libraries_get_path() to be at all meaningful.

I may be mistaken, but this patch changes the return value for non-existing libraries, so I don't see why we'd need a library anywhere to verify that the path for a non-existing library yields FALSE?

That means that for each library which doesn't have library path set we still run a useless file_exists() on the path returned by libraries_get_path(). Now it could be considered micro-optimization

File system I/O for non-existing files is slow, so we definitely should do this:

  if ($library['library path'] === FALSE || !file_exists($library['library path'])) {
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
libraries_get_patch_false_2.patch1.91 KB

Yes, that code flow is muuuch better.

How about this, then?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Fixed

Thanks for reporting, reviewing, and testing! Committed to master.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Status: Fixed » Closed (fixed)
Issue tags: -API change

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

Issue tags: +API change

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.