Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return all nodes when invoking node_load_multiple() #5437

Open
argiepiano opened this issue Jan 5, 2022 · 7 comments · May be fixed by backdrop/backdrop#3899
Open

Return all nodes when invoking node_load_multiple() #5437

argiepiano opened this issue Jan 5, 2022 · 7 comments · May be fixed by backdrop/backdrop#3899

Comments

@argiepiano
Copy link

argiepiano commented Jan 5, 2022

Description of the bug

Currently invoking node_load_multiple(); with no arguments returns an empty array. On the other hand, invoking entity_load_multiple('node'); (which is a parallel function) correctly returns all nodes as expected.

In order to get all nodes, you must invoke node_load_multiple(FALSE);.

I don't know if this is "working as designed". In my view, it would make sense for node_load_multiple() without arguments to function the same way as entity_load_multiple('node');. Plus using node_load_multiple(FALSE) to return all nodes seems somewhat contrived.

Steps To Reproduce

  1. Invoke node_load_multiple(); in a custom module or inside Devel's dpm() and output the result.

Actual behavior

The invocation returns an empty array.

Expected behavior

The invocation should return an array of all nodes in the system.

Additional information

A possible solution is to make the optional $ids parameter in node_load_multiple default to FALSE rather than to array(), which matches entity_load_multiple(). I can provide a PR.

Add any other information that could help, such as:

  • Backdrop CMS version: 1.20.3
  • Web server and its version: Apache 2.4.48
  • Operating System and its version: Mac OS 10.13
  • Browser(s) and their versions: Chrome
@argiepiano
Copy link
Author

PR backdrop/backdrop#3899

If this looks like the right approach to fix this, I can also create issues and PRs for the xxx_load_multiple of all other 4 core entities.

@argiepiano
Copy link
Author

argiepiano commented Jan 5, 2022

As a "bonus" fix we could possibly call entity_load_multiple instead of entity_load inside node_load_multiple. This would not affect functionality, but it would make more sense.

@ghost
Copy link

ghost commented Jan 5, 2022

I believe this'd be an API change and would therefore need to wait for v2.0...

@ghost ghost added this to the 2.x-future milestone Jan 5, 2022
@bugfolder
Copy link

It's a little weird that $nids is listed as optional, and then passing the default value of an empty array returns an empty array. Why would anyone ever make a call and use the default argument?

In any event, in the current version it would be nice to have the magic value of FALSE and its result documented for node_load_multiple(), as it is for entity_load_multiple().

@argiepiano
Copy link
Author

I believe this'd be an API change and would therefore need to wait for v2.0...

Hmm... I don't quite see it as an API change, but rather as a bug.

Could we at least document (in the PHP docs above the function) the unusual approach needed to get all nodes with node_load_multiple? It took me the best of one hour to figure out why node_load_multiple() did not load anything while entity_load_multiple('node') worked as expected and loaded all nodes. The need to use node_load_multiple(FALSE) to get all nodes is just not very intuitive, nor is it documented anywhere.

@kiamlaluno
Copy link
Member

kiamlaluno commented Jun 4, 2022

node_load_multiple(FALSE) returns all the nodes in the same way entity_load('node', FALSE) and entity_load_multiple('node', FALSE) do. I agree it's not an expected behavior, to use FALSE instead of an array to get all the nodes. If this is going to be changed, it should be changed in three functions.

@ghost
Copy link

ghost commented Jul 3, 2022

I don't quite see it as an API change, but rather as a bug.

Sure. But to fix the bug you need to change the API, hence the API change.

@argiepiano If you'd like this issue to be used to update the documentation for now, please create a separate issue for fixing this in 2.x. Otherwise open a new issue for updating the documentation and we can leave this issue for the 2.x fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants