Deprecate taxonomy_vocabulary_load_multiple() and taxonomy_term_load_multiple() in favour of entity_load_multiple()

It doesn't look like this patch applies anymore; it needs a re-roll.

In addition, it would be best to include the single load conversion as well.

Ok, I reworked it, I did the single load replacement.

I removed these functions but tests failed because some modules still use them, should I resubmit patch without removing these functions?

Restoring those functions.

#11: entity_load_multiple-2022509-11.patch queued for re-testing.

Fixed a bit the patch above and tested localy the tests that failed before. Seems to be ok now.

Changed the status.

Cleaned up most of the things. But I'm not sure what should I do with the hooks? Delete them?
hook_taxonomy_term_load
hook_taxonomy_vocabulary_load

and in VocabularyFormController.php 35 to 43 lines:

    $form['vid'] = array(
      '#type' => 'machine_name',
      '#default_value' => $vocabulary->id(),
      '#maxlength' => 255,
      '#machine_name' => array(
        'exists' => 'taxonomy_vocabulary_load',
        'source' => array('name'),
      ),
    );

Taking a look at removing the last unused functions.

Should the title and problem be updated to reflect the change to also include taxonomy_term_load and taxonomy_vocabulary_load?

Tried removing the functions, but we are in a situation where removing those breaks the legacy routing as it attempts to call taxonomy_term_load when there is a %taxonomy_term in a menu path - and of course the same for taxonomy_vocabulary.

I have marked the functions as deprecated and will see if I can find related issues regarding the routing system and request that they remove the load function when converting to the new routing handling.

New patch, where taxonomy_vocabulary_load_multiple() and taxonomy_term_load_multiple() functions were deleted.

#26: entity_load_multiple-2022509-26.patch queued for re-testing.

Hmm, why don't follow Islinnet's proposal in #23/#24 and deprecate taxonomy_vocabulary_load() and taxonomy_term_load()? It could then be removed at a later point as soon as no more needed by the legacy code.
Otherwise looks great, so +1!

@andypost had a talk with fubhy-the-cat earlier today regarding this and the load functions should as far as i understood on him should be considered deprecated.

They are currently only used for loading doing the legacy handling of the menu router and shouldn't be used for anything else - in that sense marking them as deprecated makes sense.

And here is the patch for it.

OK, I was thinking about the post-hook_menu era, but we probably don't want to build up more pressure and, in the end, technical debt. So fine - Rome wasn't built in a day... :)

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

It doesn't make sense to mark taxonomy_vocabulary_load() deprecated in the same patch as removing taxonomy_term_load(). Let's mark the taxonomy_term wrappers deprecated as well rather than removing them for now. The one line deprecated function isn't a lot of overhead and it's easy to remove as long as nothing in core uses it.

Catch, I think the idea is that we:
1. totally remove taxonomy_vocabulary_load_multiple() and taxonomy_term_load_multiple() functions as they're nor used any more.
2. remove all the usages taxonomy_vocabulary_load() and taxonomy_term_load() except for the one in hook_menu.
3. Mark the latter 2 functions deprecated, cause they're left for compatibility with hook_menu only.

So it seems that no one marks taxonomy_vocabulary_load() as deprecated and removes taxonomy_term_load() in the same patch.

If I misunderstood something - please clarify what did you mean?

Oh I get it now. I mis-read the patch :(

However I think we should just mark the _multiple() versions deprecated now we're past API freeze.

There's other places we'll be removing API functions still, but in those cases they're duplicating something else as opposed to simply wrapping it so mean real technical debt if they're left in (i.e. not one line). These functions were redundant in Drupal 7 too.

Here is another patch, that instead of removing "taxonomy_*_multiple" functions marks them as deprecated and deletes all their usages across the code. I think if it passes the test it's RTBC automatically?!

P.S. Of course it's up to you to decide, but I don't understand why wouldn't we want to drop the function that are not used. Is it because of API freeze? (I thought that changes are still allowed to be made by the patches that were RTBC by 1 july...)

I'm reticent to remove the function because we still have node_load_multiple(), user_load_multiple() and various others in core, which aren't even marked deprecated yet. The patch as it stands leaves us with some entity_load*() wrappers removed, some deprecated, some not across core as a whole which is introducing inconsistency rather than reducing it. Also in terms of leaving anything @deprecated in Drupal 8, one line wrappers would be the thing I'm least concerned about (compared to for example theme functions which also bloat the theme registry - i.e. they're not just a function).

There's a bit of a window for removing deprecated APIs within code freeze, but if we do that for these I'd rather do it all at once or not at all.

I agree with @catch:
we should either remove all of these functions or leave them all in.
But in any case the first step would be deprecating the functions and changing all implementations to entity_load*(). This is what #39 does, and therefore I'm very much for moving this solution forward. Didn't review the patch though.

I think I could continue and make the same changes for node_load and user_load. But first I'd like you, the core guys, to make a decision whether we should do it. Cause I wouldn't like to spend my time on something that would get declined because of API freeze/other inconsistencies.

so
1. Are we only limited to changes with node_load/node_load_multiple + user_load/user_load_multiple?
2. Should I extend this patch or just leave for D9?

P.S. Or maybe you could point me to someone whom I could ask my first question?

Marking things deprecated at this point is fine since it doesn't break any existing code at all. Doesn't have to be done here, could open separate issues.

Removal of one-line wrappers at this point I'm less keen on since they do little harm compared to a lot of other things - especially in the case of $entity_*load() functions since those are used so widely. I pinged alexpott about this issue as well to get his opinion.

At the moment I'm in favour of just deprecating all of the entity_load*() wrappers.

However, if we complete all the deprecations and have no usages in core before beta then I'm also happy to remove them at that point as we then have less functions which is a good thing. The fact they were redundant in Drupal 7 makes me more included to remove them all before beta too... they are just technical debt.

Patch looks good to me.

#39: entity_load_multiple-2022509-39.patch queued for re-testing.

Rerolled.

Have tested it, works as advertised. Code wise it looks good, haven't been able to spot any place that have been missed.

+1 for RTBC

Needs a reroll...

git ac https://drupal.org/files/2022509-delete-entity-load-48.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 31163  100 31163    0     0  20336      0  0:00:01  0:00:01 --:--:-- 23015
error: patch failed: core/modules/taxonomy/taxonomy.admin.inc:400
error: core/modules/taxonomy/taxonomy.admin.inc: patch does not apply

Rerolling... The conflict was introduced in #1946456: Convert taxonomy_term_confirm_delete() and taxonomy_vocabulary_confirm_delete() to the new form interface.

Rerolling... The conflict was introduced in #1946456: Convert taxonomy_term_confirm_delete() and taxonomy_vocabulary_confirm_delete() to the new form interface.

The most recent patch applied, so I'm removing needs reroll tag.

Back to RTBC. After patching, grep returns only the function definitions and are marked as deprecated. Tests are green.

Trasgus everywhere, removing tag.

:(

git ac https://drupal.org/files/2022509-delete-entity-load-52_0.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 30036  100 30036    0     0  19689      0  0:00:01  0:00:01 --:--:-- 22737
error: patch failed: core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/widget/TaxonomyAutocompleteWidget.php:67
error: core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/widget/TaxonomyAutocompleteWidget.php: patch does not apply

Straight reroll. Back to RTBC.

Committed/pushed to 8.x, thanks!

I was working on #2010138: Remove taxonomy_term_load_multiple() in favour of \Drupal\taxonomy\Entity\Term::loadMultiple(). and I am in agreement with comment #5 as the use of the deprecated function appears to have been eliminated from the the code. Using git blame this change appears to have occurred in commit c595d95475d74d247fdf42247255b9e5c0f494a1 which was related to this issue #2022509: Deprecate taxonomy_vocabulary_load_multiple() and taxonomy_term_load_multiple() in favour of entity_load_multiple().

So adding related issue.