Fixed: Initialize the return array in path_get_info()

[jayelless]

Description of the bug
This function will return an array of path_info data. However, this return array is built via hook_path_info() scan, followed by a hook_path_info_alter() scan, but if there are no modules implementing these hooks, then the return array is undefined. The resul is that a nul value is returned which is a problem for calling functions expecting an array to be returned.

Steps To Reproduce
===EDIT===
The original suggestion to use the contrib module membership_entity (which was in the process of porting from D7 at the time) has created an unnecessary element of confusion for this issue. An error in the module exposed the issue identified above, but it was noted that the error reported is unlikely to be encountered under normal operation of Backdrop.

However, having been discovered, it now makes sense to address the issue so that it does not become an actual problem at some future point.

There is n ow no [practical way to reporduce the problem short of remove the hook_path_info() implementations from the core modules node and user and disabling the taxonomy module. Visual inspection of the code will however identify the problem.

To reproduce the behavior:
N/A

Actual behavior
Warning: Invalid argument supplied for foreach() in path_entity_load() (line 428 of /..../web/core/modules/path/[path.module).

Expected behavior
No warning.

Additional information
Fix by adding the line
$all_info = array();
immediately following the line "if (!isset($all_info)) {" - line 598?

Also, a visual check revealed that there was a module_load_inclue call that was looking for a non-existing include file in the path module, so there is additionally a recoomendation to remove the redundant module_load_include() call
module_load_include('inc', 'path', 'path.path');

The focus of this issue is now a task to clean up the code by initialising the return array as a defensive programming move, and removing a redundant function call.

[indigoxela]

... but if there are no modules implementing these hooks, then the return array is undefined.

Nice catch!

@jayelless mind to provide a pull request?

[indigoxela]

Oops sorry, @jayelless I almost missed that you already created a PR - you did it so silently.

Two tips regarding the workflow:

1. In your PR description could you please replace This solves with Fixes, so the PR gets properly linked in this issues header and a merge would automatically close the issue?
2. If creating a PR it's always necessary to post a comment here, that you did - otherwise nobody will know that it exists

[jayelless]

Hi @indigoxela. My understanding is that the PR is linked to the issuer, so I am not sure why specific text is required in the description? I would also expect that you or someone is monitorig the PR queue, rather then looking for comments in the issue queue, to know when something is ready for testing.

However, I have updated the PR as requested.

[indigoxela]

I am not sure why specific text is required in the description?

The difference is already visible: the issue is now linked in the header and also shows up (as small icon) in the issue overview list. Additionally GitHub has special keywords that will automatically close the related issue, as soon as the PR gets merged.
That's not Backdrop specific, but GitHub specific. 😉

I would also expect that you or someone is monitorig the PR queue

There's no notification by GitHub for new PRs - that's why a comment on the issue is necessary to get the people involved notified.

[jayelless]

Hi @indigoxela. Thanks for the explanation. I will try to remember in future.

May pay to update the documentation on https://backdropcms.org/news/creating-pull-requests-against-core-easy-noob-way which states:

Give your PR a description: again, it can be pretty much anything that makes sense, but I usually add the link to the issue the PR aims to solve. This way, the PR is referenced in the issue. For the example issue mentioned above, I'd use "Fixes #2086". Note: do not input issue numbers in the usual way we reference issues in the queue (#xyz), as the PR repository and the issue queue repository are different, GitHub will link to a PR instead of the issue. Hence the suggestion to use the issue link instead.

The documentation should emphasise the need to use the text "Fixes ..." instead of making this just a vague suggestion.

[indigoxela]

The documentation should emphasise the need to use the text "Fixes ..." ...

I absolutely agree, but personally have no access to edit that content (requested a change in our Zulip chat).

[indigoxela]

I'll try to reproduce and test this soon, but two things baffle me a bit:

...install contrib module membership_entity

There's no such module. Is it a submodule of another one? UPDATE: found it.

but if there are no modules implementing these hooks...

Two required core modules implement that hook: "node" and "user". But maybe I figure out, when testing this locally, following the steps from the issue description.

[indigoxela]

@jayelless OK, found the module - it's not in backdrop-contrib yet (GitHub search hooray).

Hm. Following your instructions I get a WSOD (http 500) and on next page call a bunch of Notice/Warning messages in step 5 (create memberships):

    Notice: Trying to access array offset on value of type null in membership_entity_update_alias() (line 443 of /var/www/bdroptesting/html/modules/membership_entity/membership_entity.module).
    Notice: Trying to access array offset on value of type null in membership_entity_update_alias() (line 443 of /var/www/bdroptesting/html/modules/membership_entity/membership_entity.module).
    Warning: require_once(/var/www/bdroptesting/html/modules/entity_plus/views/handlers/entity_plus_views_handler_field_boolian.inc): failed to open stream: No such file or directory in require_once() (line 4022 of /var/www/bdroptesting/html/core/includes/bootstrap.inc).

I took branch 1.x-1.x from there: https://github.com/jayelless/membership_entity Is that wrong?

Note: I'm unfamiliar with that module, never used it before.

What I do not see is the message you posted in the issue description.

The third one is simply a typo in your code: boolian vs. boolean. 😁 i != e UPDATE - that typo is in entity_plus!

The other two refer to following code:

  $uri = entity_uri('membership_entity', $membership);
  if (!module_exists('pathauto')) {
    return $uri['path'];
  }

A module named "pathauto" doesn't exist in Backdrop (actually it's deprecated). The core module's called path - that's why this if clause needs to get updated. And all the other "pathauto", too.

Besides that the code still contains something like ctools_plugin_get_class() - ctools doesn't exist at all in Backdrop.

There might be an actual problem in core, but I'd recommend to first fix the module (which, to me, looks like a beast to port!).
As soon as the most obvious problems in the module got fixed - if there's still a problem - let's proceed here.

[jayelless]

@indigoxela You got the correct branch of the module. I am waiting to get approval to transfer it to the Backdrop contrib space.

Re ctools. The membership_entity module was heavily dependent on ctools in D7, and while I have removed/refactored most of the calls, there are still some to do. There is an include file "membership_entity.ctools.inc" that gets loaded and which contains the few remaining ctools functions (copied from the original ctools module). There is no dependency on ctools itself as the include file is completely self-contained.

The reference to pathauto likewise is only triggered if the module exists. It does not exist, so the relevant code is never executed.

Did you configure the membership_entity settings? The errors you report look like some settings do not exist? I will re-confirm the problem.

[indigoxela]

Did you configure the membership_entity settings?

Yes I did. The membership entity gets created, BTW.

Please add two essential infos to the issue description:

1. A link to the module
2. That it's essential to use the latest dev of entity_plus

And eventually add more info about the setup of membership_entity. You shouldn't assume that people know all that out-of-the-box.

I still doubt that your assumption about the root cause of your problem is plausible. As I already mentioned: two required core modules implement hook_path_info(), so to my understanding the result can never be NULL, unless there's some mistake elsewhere.

[indigoxela]

Here's my finding:

In membership_entity.inc there's an empty function uri() with a pretty odd comment:

  /**
   * Returns the URI elements of the entity.
   *
   * @return
   *   An array containing the 'path' and 'options' keys used to build the URI
   *   of the entity, and matching the signature of url(). NULL if the entity
   *   has no URI of its own.
   */
  public function uri() {

  }

If I let the function actually return something - like the comment above claims, then everything works as expected, the action links appear:

  public function uri() {
    return array(
      'path' => 'membership/' . $this->mid,
      'options' => array(),
    );
  }

So it still looks like the problems should get fixed in the contrib module.
https://github.com/backdrop-contrib/membership_entity/blob/1.x-1.x/membership_entity.inc#L127

[jayelless]

Hi @indigoxela Thanks for the analysis of the problems with the module membership_entity. While you have identified some issues, these are in fact not causing the bug in the path module which I reported to show up.

The actual problem was in hook_autoload_info in the sub-module membership_entity_type. This was the result of an include file being loaded twice, causing the evaluation of modules implementing hook_path_info to terminate prematurely. Thus, the system was working on the belief that there were no modules implementing this hook, which exposed the bug in the path module.

This issue report is not about the membership_entity module, so we can forget about the problems present in that, and look at the problem present in the path module.

I have identified an additional problem in the path module in the same function path_get_info() with the line
module_load_include('inc', 'path', 'path.path');
which is trying to load a non-existing file ("path.path.inc").

The original problem I identified is still present, and while it is not encountered under normal operations, it is good defensive practice to initialise return elements where there is a possibility they will not be defined in subsequent execution of the code. It may be possible that future changes to the node, user, and taxonomy modules might remove the current implementations of hook_path_info, which would cause this error to show up.

I have updated the change and redone the PR for consideration.

[indigoxela]

while it is not encountered under normal operations, it is good defensive practice to initialise return elements where there is a possibility they will not be defined

Sure, valid point. Let's get some more feedback on that.

[indigoxela]

Regarding the "additional bug" - you now removed that module_load_include(). Uhoh... looks weird.

But let's get some feedback regarding the "defensive code" topic first.

[jayelless]

Sure. Would be good to get some comments on the changes I have proposed. Possibly the module_load_include was there for a reason, but it was doing nothing at present.

[indigoxela]

Possibly the module_load_include was there for a reason, but it was doing nothing at present.

I did a quick git blame - that code's around for a while. No, there's no file path.path.inc in the path module, so this line is redundant, probably a left-over from restructuring when the module went into core.

[indigoxela]

One thing worries me a bit, we now have 16 comments in this issue, and most of them are about something different than the main topic here. Usually people get confused by something like that.

At least, could you please update the issue description to better reflect the (now slightly changed) topic? So people don't have to read all the unrelated comments, but get the problem at a glance.

• Defensive and cleaner code
• Cleanup of redundant (left-over) code line

[jayelless]

Thanks @indigoxela. I have updated the issue description.

[jayelless]

Following a review of issue #5142, I think a better solution to this issue would be to update the code to:

$all_info = &backdrop_static(__FUNCTION__, array());

  if (empty($all_info)) {
    $modules = module_implements('path_info');

which both addresses the missing "&" on the backdrop_static function call, and initialises the return array via the backdrop_static call. The redundant call to module_load_include(0 has also been remvoed.

[indigoxela]

... I think a better solution to this issue would be to update the code to ... backdrop_static ...

@jayelless does that mean, you'd prefer to completely close this issue in favor of the other one? Or do only the removal of the pointless module_load_include here?

[herbdool]

I suggest making the change here since it's more than just adding an ampersand.

[bugfolder]

Following a review of issue #5142, I think a better solution to this issue would be to update the code to:
$all_info = &backdrop_static(__FUNCTION__, array());

In theory, initializing $all_info to an empty array and then doing an empty() check on it would result in the rebuild loop getting called every time if $all_info is legitimately empty after the rebuild loop. But since both node and taxonomy core modules implement hook_path_info(), $all_info can't be legitimately empty, so this should be OK.

[bugfolder]

Agree with @herbdool, remove the pointless module_load_include here and put in the missing ampersand. I'm preparing a PR for #5142 and won't include this ampersand in that PR.

Edit: it's backdrop/backdrop#3678.

[jayelless]

I have updated the PR as proposed. The call to backdrop_static will initialise the return array if required, and the redundant call to module_load_include has been removed.

[indigoxela]

I have updated the PR as proposed.

If you also add the ampersand, we're done here. 😉

[jayelless]

My bad :(. Ampersand added now and new commit pushed.

[indigoxela]

Ampersand added now

Code looks great now. Many thanks for cleaning up that "dusty corner" (and initiating another clean up task).
RTBC from my side. 👍 (there's not much to test here)

[quicksketch]

Hi folks, this took me a while to read the issue and get a grasp on the problem, so I wasn't able to get it included in the 1.19.3 release. However, after reading through this I think concerns have been addressed. I merged backdrop/backdrop#3673 into 1.x and 1.19.x. Thanks!