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

Add rule for node_load_multiple #148

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jedihe
Copy link

@jedihe jedihe commented May 21, 2021

node_load_multiple() can be properly refactored by tweaking the existing logic in EntityLoadBase.

I tried improving it by using a protected variable ($isMultiple), defaulting it to FALSE in EntityLoadBase and setting it to TRUE in NodeLoadMultipleRector, as well as updating EntityLoadBase to now use it to determine $method_name, etc. For some reason, that change caused the is_null() check for $this->entityType to start returning TRUE, so I just left those changes out of the PR.

I also ran a quick sanity check by diffing a processed copy of rector_examples/node_load.php vs. the _updated version, which showed no changes at all.

* Improvement opportunities
* - See EntityLoadBase.php
*/
final class NodeLoadMultipleRector extends EntityLoadBase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it will duplicate / be redundant to the implementation of https://github.com/palantirnet/drupal-rector/blob/master/src/Rector/Deprecation/NodeLoadRector.php.

Copy link
Contributor

@damontgomery damontgomery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

I'm not sure how to move forward. I think it's a good addition to support the multiple loading.

One of the guiding principles of the project is to have each class do a single thing and this introduces a class / set of classes that does two things. I think that could lead to confusion. It avoids copy & paste, but adds some complexity. It's not a lot, but I would tend towards creating an EntityLoadMultipleBase alongside EntityLoadBase.

The NodeLoadMultipleRector class is an example of this confusion. I think it duplicates NodeLoadRector, so there are two classes that do the same thing, which could be confusing.

Aside: We also need to update the index file deprecation-index.yml.

I want to be clear that I'm not saying these are requirements. They are suggestions. :)

Thanks again and happy to have other community feedback.

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

Successfully merging this pull request may close these issues.

3 participants