Problem/Motivation

In #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities the static methods Entity::load() and Entity::loadMultiple() are implemented.

Beta phase evaluation

#2321701-15: Replace all instances of contact_form_load(), entity_load('contact_form') and entity_load_multiple('contact_form') with static method calls @alexpott mentions it is allowed since it is removal of deprecated code.
It might be that not all child issues are as simple as that.

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is refactoring code, using new APIs. per https://www.drupal.org/core/issue-category
Issue priority Major because it is across many systems, but only code refactoring. Not critical because it is a task, and nothing is actually broken. But each child issue is most likely Normal (just in one system). per https://www.drupal.org/core/issue-priority
Unfrozen changes It is*not* unfrozen only changes.
Prioritized changes The main goal of this issue is removing previously deprecated code.
Disruption Refactoring usages of the deprecated code in Core is not disruptive for contributed and custom modules/themes because it will NOT require a BC break in contrib. The deprecated functions are not themselves being removed in children of this meta, just usages of them changed to be using what they are supposed to be using.

This seems to be allowed. But individual children might have greater disruption so should be looked at.

Proposed resolution

We need to replace all calls to ENTITY_TYPE_load() and ENTITY_TYPE_load_multiple() with these new static method calls. An example:

Before:

  // Variants for loading a single node entity.
  $node = node_load($nid);
  $node = entity_load('node', $nid);

  // Variants for loading multiple node entities.
  $nodes = node_load_multiple($nids);
  $nodes = entity_load_multiple($nids);

  // Clear the entity cache for a bunch of nodes.
  node_load_multiple($nids, TRUE);

After:

  use Drupal\node\Entity\Node;

  // Load a single node entity.
  $node = Node::load($nid);

  // Load multiple node entities.
  $nodes = Node::loadMultiple($nids);

  // Clear the entity cache for a bunch of nodes.
  \Drupal::entityManager()->getStorageController('node')->resetCache($nids);

It is a good candidate for the function replace script, see #2208607: Write script to automate replacement of deprecated functions where possible

Remaining tasks

User interface changes

No.

API changes

No.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ianthomas_uk’s picture

pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

ianthomas_uk’s picture

Status: Postponed » Active
m1r1k’s picture

Issue tags: +#ams2014contest
rpayanm’s picture

Assigned: Unassigned » rpayanm
Issue tags: -
rpayanm’s picture

I have some doubts and I need you to explain them

1.
Before:

$feed = entity_load('aggregator_feed', $fid);

After:

$feed = Feed::load($fid);

And for this?
Before

$feed = entity_load('aggregator_feed', $fid, TRUE);

After:

?

2. For Comment Entity, I have:
Before:

$this->comments = entity_load_multiple('comment', $cids);

To change should be:

Comment::load($cids);

But there is a method:

loadMultiple($cids);

So my question is: Which is correct?

Greetings.

pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

Assigned: rpayanm » Unassigned

@rpayanm, you can use Comment::loadMultiple() to load multiple items. This was wrong in the summary, I have fixed it now, thanks for mentioning it!

I have unassigned you from this issue. Assigning an issue to yourself means "I'm working on this, please don't touch!". But since this is a meta issue the actual work need to be done in the child issues, so it does not make much sense to have this assigned to someone. If you are working on one of the child issues, feel free to assign them to yourself!

rpayanm’s picture

Hello:

Thank you very much for the reply.

I am a new contributor, sorry :)

Then the first question which is the answer?

Greetings.

Berdir’s picture

That is a bit more complicated now, you need to do something like this as a separate call, first:

\Drupal::entityManager()->getStorage('comment')->resetCache();

If you just want to delete a specific comment from the cache, you can also do this:

\Drupal::entityManager()->getStorage('comment')->resetCache(array($cid));

But in tests, it usually doesn't matter. And those calls should all be in tests.

We explicitly didn't add this back to the new load methods, because it's something that is only needed for tests and people shouldn't call that just in case or so in real code, as it would be bad for performance.

mradcliffe’s picture

Crell caught that we should be turning these into services.

We should do this in a follow-up issue since several of the child issues have been fixed.

vincenzodb’s picture

Assigned: Unassigned » vincenzodb
vincenzodb’s picture

Issue tags: +#drupalsprintIT
vincenzodb’s picture

Added replacement for entity_load_multiple() calls.

vincenzodb’s picture

Status: Active » Needs review
vincenzodb’s picture

I'm working on entity_load() calls

Status: Needs review » Needs work

The last submitted patch, 15: drupal_core-replace_entity_load_multiple-2225347-15.patch, failed testing.

YesCT’s picture

@vincenzodb This is a meta issue, so I was not expecting a patch here.

To work on one of the types that already has an issue, find a child issue (in the sidebar).

To work on a type that doesn't have a child yet...
Pick one of the child issues in the sidebar, and create a similar issue for each type, (with dreditor clone button and then change the values, or just create a new issue and fill in all the values).

For the list in the summary, you could see if there is already an issue and add the issue to that list item. (Note issues can be linked in the summary and comments with [#NNNNN] where NNNN is the issue number.

Once all the children are created and fixed, then we will close this meta.

YesCT’s picture

Priority: Normal » Major
Issue summary: View changes

updated the issue summary for allowed in beta, and seems to be allowed because removing/refactoring usages of deprecated code. (but not removing the deprecated functions themselves, so no BC break)

YesCT’s picture

Assigned: vincenzodb » Unassigned

unassigning since work should be done in child issues (also see #9)

YesCT’s picture

Issue summary: View changes

updating the list in the summary matching with child issues. next need to search if there are more child issues existing, but just were not linked to this parent.

YesCT’s picture

Issue summary: View changes

made some of the children for what was in that patch. Not sure if should have a child for each of the types though, so will wait a bit for feedback before making them all.

YesCT’s picture

Issue tags: +SprintWeekend2015Queue

Identifying this as a meta with children that have a potential good issue for Sprint Weekend. See discussion at #2407325: preparing for a sprint, would be good to have a list of candidate issues. Adding sprint queue tag.
If any child is worked on during the sprint, please add the tag SprintWeekend2015 to the child. Add a comment on the child when you start work saying what you are about to do, so we can coordinate.

DevElCuy’s picture

DevElCuy’s picture

Issue tags: +SprintWeekend2015Queue

Removed SprintWeekend2015Queue by mistake.

xjm’s picture

So there's been lots of great work on this, and it looks like most of the child issues are complete.

However, these changes I think aren't quite right in terms of the prioritized changes list, which specifies:

code already marked for removal by 8.0.0

For #2322195: Replace all instances of user_load(), user_load_multiple(), entity_load('user') and entity_load_multiple('user') with static method calls, the code is marked for removal by 9.0, so we can't remove it, which makes removing usages of it not a prioritized change. And for #2321901: Replace all instances of entity_load('image_style') and entity_load_multiple('image_style') with static method calls and similar, entity_load() and entity_load_multiple() aren't deprecated at all.

However, we could decide that since the work is mostly done here (and this issue is considered major after all), it's better to finish the last few and just retain the deprecated wrappers. I might recommend that (as well as verifying that all of these get deprecated for 9.0).

However, this does not extend to other deprecations that are not part of this issue. We should only be doing conversions to remove calls to things deprecated for 8.0.0 in general.

xjm’s picture

Status: Needs work » Active

Also the NW status on a meta was confusing me. :)

alexpott’s picture

i think if we have a function that is deprecated for 9.0.0 it is good to remove usages. And I don't think that is it that disruptive to do this during the beta.

Wrt to entity_load and entity_load_multiple I think we should deprecate them for 9.0.0 as well since we should be removing them in that release as we plan to remove .module files.

andypost’s picture

andypost’s picture

The issue #2465751: Remove config_test_load() and replace with entity_load shows how fragile the static approach could be.
config_test_entity_type_alter() adds config_test_no_status entity type without class and that leads to AmbiguousEntityClassException from \Drupal\Core\Entity\EntityManager::getEntityTypeFromClass()

bojanz’s picture

Mile23’s picture

Is this meta done or what? :-)

#2465751: Remove config_test_load() and replace with entity_load is the only child remaining, but it replaces a function not marked as deprecated. However, it illustrates best practices and removes ENTITY_load(). Also, the work is done and RTBC.

JeroenT’s picture

Status: Active » Fixed

#2465751: Remove config_test_load() and replace with entity_load Is also in, so all the work is done here.

Status: Fixed » Closed (fixed)

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