Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Temoor’s picture

Status: Active » Needs review
FileSize
24.33 KB
Michael Hodge Jr’s picture

Status: Needs review » Reviewed & tested by the community

I've applied the patch, grepped the code, and can confirm everything looks as it should. Setting to RTBC.

Temoor’s picture

Replaced #machine_name callback in VocabularyForm.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/taxonomy/src/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
    @@ -13,6 +13,7 @@
    +use Drupal\taxonomy\Entity\Vocabulary;
    
    @@ -81,7 +82,7 @@ public function getSettableOptions(AccountInterface $account = NULL) {
    -        if ($vocabulary = entity_load('taxonomy_vocabulary', $tree['vocabulary'])) {
    +        if ($vocabulary = Vocabulary::load($tree['vocabulary'])) {
    
    @@ -121,7 +122,7 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
    -    $vocabularies = entity_load_multiple('taxonomy_vocabulary');
    +    $vocabularies = Vocabulary::loadMultiple();
    
    +++ b/core/modules/taxonomy/src/Plugin/Field/FieldWidget/TaxonomyAutocompleteWidget.php
    @@ -10,6 +10,7 @@
    +use Drupal\taxonomy\Entity\Vocabulary;
    
    @@ -104,7 +105,7 @@ public function massageFormValues(array $values, array $form, FormStateInterface
    -      if ($vocabulary = entity_load('taxonomy_vocabulary', $tree['vocabulary'])) {
    +      if ($vocabulary = Vocabulary::load($tree['vocabulary'])) {
    
    +++ b/core/modules/taxonomy/src/Plugin/entity_reference/selection/TermSelection.php
    @@ -11,6 +11,7 @@
    +use Drupal\taxonomy\Entity\Vocabulary;
    
    @@ -63,7 +64,7 @@ public function getReferenceableEntities($match = NULL, $match_operator = 'CONTA
    -      if ($vocabulary = entity_load('taxonomy_vocabulary', $bundle)) {
    +      if ($vocabulary = Vocabulary::load($bundle)) {
    
    +++ b/core/modules/taxonomy/src/Plugin/views/argument/VocabularyVid.php
    @@ -9,6 +9,7 @@
    +use Drupal\taxonomy\Entity\Vocabulary;
    
    @@ -23,7 +24,7 @@ class VocabularyVid extends Numeric {
    -    $vocabulary = entity_load('taxonomy_vocabulary', $this->argument);
    +    $vocabulary = Vocabulary::load($this->argument);
    
    +++ b/core/modules/taxonomy/src/Plugin/views/argument_default/Tid.php
    @@ -15,6 +15,7 @@
    +use Drupal\taxonomy\Entity\Vocabulary;
    
    @@ -80,7 +81,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    -    $vocabularies = entity_load_multiple('taxonomy_vocabulary');
    +    $vocabularies = Vocabulary::loadMultiple();
    
    +++ b/core/modules/taxonomy/src/Plugin/views/field/TaxonomyIndexTid.php
    @@ -12,6 +12,7 @@
    +use Drupal\taxonomy\Entity\Vocabulary;
    
    @@ -64,7 +65,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    -    $vocabularies = entity_load_multiple('taxonomy_vocabulary');
    +    $vocabularies = Vocabulary::loadMultiple();
    
    @@ -93,7 +94,7 @@ public function query() {
    -    $vocabularies = entity_load_multiple('taxonomy_vocabulary');
    +    $vocabularies = Vocabulary::loadMultiple();
    
    +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
    @@ -14,6 +14,7 @@
    +use Drupal\taxonomy\Entity\Vocabulary;
    
    @@ -55,7 +56,7 @@ protected function defineOptions() {
    -    $vocabularies = entity_load_multiple('taxonomy_vocabulary');
    +    $vocabularies = Vocabulary::loadMultiple();
    
    @@ -99,7 +100,7 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
    -    $vocabulary = entity_load('taxonomy_vocabulary', $this->options['vid']);
    +    $vocabulary = Vocabulary::load($this->options['vid']);
    
    +++ b/core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData.php
    @@ -11,6 +11,7 @@
    +use Drupal\taxonomy\Entity\Vocabulary;
    
    @@ -46,7 +47,7 @@ protected function defineOptions() {
    -    $vocabularies = entity_load_multiple('taxonomy_vocabulary');
    +    $vocabularies = Vocabulary::loadMultiple();
    

    Inject the storage into the plugins

  2. +++ b/core/modules/taxonomy/src/VocabularyForm.php
    @@ -41,7 +41,7 @@ public function form(array $form, FormStateInterface $form_state) {
    -        'exists' => 'taxonomy_vocabulary_load',
    +        'exists' => ['Drupal\taxonomy\Entity\Vocabulary', 'load'],
    

    This should be done through dependency injection too. See ActionFormBase for an example.

Upchuk’s picture

Assigned: Unassigned » Upchuk
Issue tags: -

Patch needs reroll and will attempt to fix the remaining issue raised in #4.

Upchuk’s picture

The entity_reference TermSelection plugin uses ReflectionFactory which does not use the container. Fix proposed in #2107243: Decouple entity reference selection plugins from field definitions

Upchuk’s picture

FieldType plugins are using TypedData factories so no dependency injection. See #2053415: Allow typed data plugins to receive injected dependencies

Upchuk’s picture

Attaching txt with reroll (please review), an interdiff since the reroll and the patch which contains the storage injections.

The patch is missing the storage injection for the following cases:

FieldType plugin mentioned in #7
TermSelection plugin mentioned in #6
VocabularyVid views argument plugin that I'm not sure it's being used. See #2048733: Taxonomy vocabulary contextual filters are missing

seanB’s picture

Patch in #8 applied perfectly.

Found a extra call to entity_load('taxonomy_vocabulary') though, in /core/modules/views_ui/admin.inc on line 233.

Patch with that last change is attached.

Status: Needs review » Needs work
Upchuk’s picture

Assigned: Upchuk » Unassigned

Hey Sean,

Thanks for the patch, however it failed testing.

Can you please resubmit and include an interdiff of the work you put on top of #8 so we can see what you worked on?

I unassigned this issue now so you can assign it to yourself while you do that. After submitting the patch, you can unassign it again.

Thanks!

seanB’s picture

Sorry about that.. I'll do this later today!

Status: Needs work » Needs review
seanB’s picture

FileSize
913 bytes

Here is the interdiff for #9.

rpayanm’s picture

Issue tags: +Needs reroll

Status: Needs review » Needs work
jamesdixon’s picture

Status: Needs work » Needs review
FileSize
33.09 KB

Here's my attempt at a reroll.

There were differences in bringing classes in via use since the last patch at the top of some files that needed resolving.

The patchutils interdiff was giving me the same error as reported here: https://fedorahosted.org/patchutils/ticket/23. Any good workarounds to that?

rpayanm’s picture

Status: Needs review » Reviewed & tested by the community

Look fine for me :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs reroll
+++ b/core/modules/taxonomy/src/Plugin/Field/FieldWidget/TaxonomyAutocompleteWidget.php
@@ -15,6 +15,7 @@
+use Drupal\taxonomy\Entity\Vocabulary;

@@ -137,7 +138,7 @@ public function massageFormValues(array $values, array $form, FormStateInterface
-      if ($vocabulary = entity_load('taxonomy_vocabulary', $tree['vocabulary'])) {
+      if ($vocabulary = Vocabulary::load($tree['vocabulary'])) {

+++ b/core/modules/taxonomy/src/Plugin/views/argument/VocabularyVid.php
@@ -9,6 +9,7 @@
+use Drupal\taxonomy\Entity\Vocabulary;

@@ -23,7 +24,7 @@ class VocabularyVid extends Numeric {
-    $vocabulary = entity_load('taxonomy_vocabulary', $this->argument);
+    $vocabulary = Vocabulary::load($this->argument);

I think we should be injecting the vocabulary storage for these two.

rosinegrean’s picture

Status: Needs work » Needs review
FileSize
4.68 KB
35.72 KB
skipyT’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/taxonomy/src/Plugin/Field/FieldWidget/TaxonomyAutocompleteWidget.php
    @@ -36,12 +36,18 @@ class TaxonomyAutocompleteWidget extends WidgetBase implements ContainerFactoryP
    +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, array $third_party_settings, EntityStorageInterface $term_storage, VocabularyStorageInterface $vocabulary_interface) {
    

    You should rename this variable to $vocabulary_storage, we are injection here the storage object, not the interface

  2. +++ b/core/modules/taxonomy/src/VocabularyForm.php
    @@ -18,6 +20,32 @@
    +  protected $storage;
    

    this one also should be $vocabularyStorage instead of $storage

  3. +++ b/core/modules/taxonomy/src/VocabularyForm.php
    @@ -18,6 +20,32 @@
    +  public function __construct(VocabularyStorageInterface $storage) {
    

    lets rename this $vocabulary_storage

skipyT’s picture

Other stuff I missed before:

  1. +++ b/core/modules/taxonomy/src/Plugin/Field/FieldWidget/TaxonomyAutocompleteWidget.php
    @@ -36,12 +36,18 @@ class TaxonomyAutocompleteWidget extends WidgetBase implements ContainerFactoryP
    +  protected $vocabularyStorage;
    

    Here we need a comment like: "The Vocabulary storage."

  2. +++ b/core/modules/taxonomy/src/Plugin/views/argument/VocabularyVid.php
    @@ -20,10 +22,46 @@
    +   * @param VocabularyStorageInterface $vocabulary_storage
    +   *   The vocabulary storage interface.
    

    The comment should be only: The vocabulary storage.

rosinegrean’s picture

Status: Needs work » Needs review
FileSize
3.48 KB
35.8 KB
skipyT’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
    $vocabulary = entity_load('taxonomy_vocabulary', $vocabulary->id());

in core/modules/system/src/Tests/Entity/EntityCrudHookTest.php needs converting too.

pcambra’s picture

FileSize
33.08 KB
aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 2322105-28.patch, failed testing.

rpayanm’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
33.15 KB

rerolled

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: 2322105-31.patch, failed testing.

rpayanm’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
33.18 KB

rerolled

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 33: 2322105-33.patch, failed testing.

rpayanm’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
33.38 KB

rerolled

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3b53231 and pushed to 8.0.x. Thanks!

Beta evaluation is in the meta issue.

  • alexpott committed 3b53231 on 8.0.x
    Issue #2322105 by rpayanm, prics, Upchuk, Temoor, seanB, pcambra,...
kim.pepper’s picture

Status: Fixed » Needs review
FileSize
617 bytes

Quick follow up:

This patch added an unknown and unused interface use Drupal\Core\TypedData\AllowedValuesInterface

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

good find

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed follow-up to 8.0.x. Thanks!

  • webchick committed 1d454b9 on 8.0.x
    Issue #2322105 follow-up by kim.pepper: Removed unused "use" statement.
    

Status: Fixed » Closed (fixed)

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