Deprecate some procedural functions in taxonomy.module

This makes sense to me.

error: core/modules/taxonomy/tests/src/Functional/VocabularyCrudTest.php: does not exist in index

1. +++ b/core/includes/bootstrap.inc
   @@ -950,6 +950,10 @@ function &drupal_static($name, $default_value = NULL, $reset = FALSE) {
   +    return;
   

   The return here is not correct. We should still continue with the original behaviour even though it is pointless. Deprecations should by and large not change behaviour.

2. +++ b/core/modules/taxonomy/taxonomy.module
   @@ -337,8 +334,7 @@ function taxonomy_vocabulary_get_names() {
      if (isset($vocabulary)) {
   -    $vocabularies = taxonomy_vocabulary_get_names();
   -    if (isset($vocabularies[$vocabulary])) {
   +    if (\Drupal::entityTypeManager()->getStorage('taxonomy_vocabulary')->load($vocabulary)) {
          $values['vid'] = $vocabulary;
        }
        else {
   

   I don't get why we both checking for the existence of the vocabulary. We don't save a query because a non-existent vocab will need a query to work out it doesn't exist and if we only did:

     if (isset($vocabulary)) {
       $values['vid'] = $vocabulary;
     }
   

   We'd still get an empty array and the taxonomy_term_data table has a key on vid so the query will be quick if it doesn't exist.

I made the changes on points #6.1 and #6.2 (even if I'm not sure about point 2).
The patch has also been rerolled, as VocabularyCrudTest has been moved from Functional to Kernel test.

Changing deprecation message from Drupal 8.8.0 and will be removed before Drupal 9.0.0 to Drupal 9.1.0 and will be removed before Drupal 10.0.0.

Sorry but the deprecation needs updating again.

RTBC for these changes, +1 to using a switch in drupal_static_reset.

I think all the legacy code in the argument validator plugins can be removed, I can't see how it can ever work, but that's for a followup (or an issue might already exist).

Opened #3220647: Remove "legacy vids" handling from taxonomy Views plugins as followup

Big fan of this issue. This is a thing I wanted cleaned up as a subsystem maintainer for Taxonomy (but the whole release management thing got in the way). A few nitpicks and a couple questions:

1. +++ b/core/includes/bootstrap.inc
   @@ -614,6 +614,9 @@ function &drupal_static($name, $default_value = NULL, $reset = FALSE) {
   +    @trigger_error("Using drupal_static_reset() with 'taxonomy_vocabulary_get_names' as parameter is deprecated in Drupal 9.1.0 and will be removed before Drupal 10.0.0. There's no replacement for this usage. See https://www.drupal.org/node/3039041.", E_USER_DEPRECATED);
   

   We're at 9.3 now.

   Also, we normally avoid contractions in core text (so "There is" rather than "There's").

2. +++ b/core/modules/taxonomy/src/VocabularyStorage.php
   @@ -9,14 +9,6 @@
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public function resetCache(array $ids = NULL) {
   -    drupal_static_reset('taxonomy_vocabulary_get_names');
   -    parent::resetCache($ids);
   -  }
   -
   

   Hmm, is it safe to just rip out this method implementation?

3. +++ b/core/modules/taxonomy/taxonomy.module
   @@ -211,20 +211,17 @@ function taxonomy_vocabulary_static_reset(array $ids = NULL) {
   + * @deprecated in Drupal 9.1.0 and will be removed before Drupal 10.0.0. Use
   ...
   +  @trigger_error('taxonomy_vocabulary_get_names() is deprecated in Drupal 9.1.0 and will be removed before Drupal 10.0.0. Use \Drupal::entityTypeManager()->getStorage("taxonomy_vocabulary")->loadMultiple() instead, to get a list of vocabulary entities keyed by vocabulary ID. See https://www.drupal.org/node/3039041.', E_USER_DEPRECATED);
   

   Ditto on updating this to say 9.3.

4. +++ b/core/modules/taxonomy/taxonomy.module
   @@ -244,14 +241,7 @@ function taxonomy_vocabulary_get_names() {
   -    else {
   -      // Return an empty array when filtering by a non-existing vocabulary.
   -      return [];
   -    }
   

   What's happening to this code path? Does this case have test coverage?

5. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -92,11 +92,6 @@ public function testTaxonomyVocabularyLoadMultiple() {
   -    // Fetch the names for all vocabularies, confirm that they are keyed by
   -    // machine name.
   -    $names = taxonomy_vocabulary_get_names();
   -    $this->assertEquals($vocabulary1->id(), $names[$vocabulary1->id()]);
   

   This test coverage should maybe be moved to a legacy test?
    

6. Finally, I think the CR should also mention the bit about the static cache.

Thanks for working on this!

Makes sense to me to deprecate taxonomy_term_load_multiple_by_name() here as well.

@claudiu.cristea if they don't have usages (or just one or two), then it makes sense to me to deprecate them in one issue - it's unlikely to conflict with other patches, a single change record will make things easier to find etc.

We are using the MR in this issue.

That "legacy" code in the Views handlers was removed from core in #1552396: Convert vocabularies into configuration but accidentally reintroduced in #1850792-26: Make init() method consistent across all views plugins. Because of that isset() check, it has been dead code this entire time, and the whole init() method can be removed in both cases.

Re: #32, I believe @tim.plunkett is referring to @longwave's comment here.

I think cleaning that up could be a followup issue? Since replacing the prodedural functions with entity API calls are 1:1 changes, but removing dead code in views handlers is a different logical scope.

I opened a follow-up here: #3221149: Remove dead code in Taxonomy's Views handlers
With that out of the way, the rest of the patch seems fine. Thanks!

NW for the above point about the scope creep of rewriting the legacy test vs. the original one. They should be kept as similar as possible. Thanks!

Thanks @claudiu.cristea, the test changes are so much more straightforward now. I like that you kept the new test additions too.

I think that #3221149: Remove dead code in Taxonomy's Views handlers should land before this patch. It removes all runtime usages of taxonomy_term_load_multiple_by_name() except for taxonomy_term_load_multiple_by_name() (which itself is only called by test code) and makes the performance impact of this patch non-existent. If this patch lands first it will result in additional queries because we're calling \Drupal::entityQuery('taxonomy_vocabulary')->execute(); instead of taxonomy_vocabulary_get_names(). The old code implements a static cache whereas entity queries are uncached and results in additional queries to the config table.

This issue is RTBC. That one is not. Either is easy to reroll. Therefore, since I think this is also committable, I'm going to go ahead and commit this despite #39.

Committed to 9.3.x and published the CR. Thanks!

@pguillard, in the future, please include interdiffs when updating patches. (For merge requests, they are not needed.) Thanks!