backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

Update documentation for `node_load_multiple(FALSE)`. #5889

Closed avpaderno closed 1 year ago

avpaderno commented 1 year ago

As reported in #5437, calling node_load_multiple() with no arguments returns an empty array, while entity_load_multiple('node'), the function called by node_load_multiple()) returns all nodes as expected.

This happens because the default value for entity_load_multiple()'s $ids is FALSE, while the default value for node_load_multiple()'s $nids is array().

$nids should be documented to accept FALSE as value, which is the value used to return all nodes.

avpaderno commented 1 year ago

The failing tests are for blocks that should appear on a page.

ghost commented 1 year ago

@kiamlaluno It's really hard to review your PR when there are so many unrelated changes. I know they're mostly for fixing coding standard issues, but I think they need to be reviewed properly in their own issue.

Can you please take out changes unrelated to this issue, and perhaps open a new issue to fix the issues in core/modules/node/node.module?

avpaderno commented 1 year ago

@BWPanda I will. I guess that when I created this PR I thought I had to also fix the coding standard issues in the changed file.

ghost commented 1 year ago

Nah. Eventually all existing CS issues will be fixed throughout core, so when the CS action is run on a PR it'll only flag issues with the code that was changed. But until then, it'll flag a bunch of stuff that can be ignored for now. It'd be great to get it fixed as part of an existing PR, but when it makes reviewing the PR difficult then I think a separate issue is necessary.

avpaderno commented 1 year ago

I just added type-hints for node_load_multiple(). I hope that is fine.

avpaderno commented 1 year ago

(I need to remember that the pull-down menu does not remember the labels removed directly from the page.)

argiepiano commented 1 year ago

@kiamlaluno I left a small comment in the PR. Other than that LGMT.

BTW I've noticed that phpcs complains more if you are working from an older version of the dev branch. Rebasing often takes care of most complaints.

argiepiano commented 1 year ago

LGTM.

What type of "milestone" label do documentation changes get?

bugfolder commented 1 year ago

Documentation change to describe existing behavior would be a minor milestone (which I've added).

avpaderno commented 1 year ago

I rebased the PR. It does not seem it helped with phpcs complains, though. (It must be my usual luck. 😅)

argiepiano commented 1 year ago

Documentation change to describe existing behavior would be a minor milestone (which I've added).

If it's minor for the milestone, shouldn't it then be 1.25.0?

bugfolder commented 1 year ago

Brain-fart. I mean bugfix release.

bugfolder commented 1 year ago

Sorry. Features (new features) get minor. Bugfixes and tasks (this was a task) get bugfix release.

ghost commented 1 year ago

Added a suggestion in the PR.

quicksketch commented 1 year ago

Merged into 1.x and 1.24.x. Thanks folks!