QueryFactory is deprecated. We should use EntityTypeManager

Ref. https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

CommentFileSizeAuthor
#6 interdiff.txt3.19 KBmile23
#6 2897889_6.patch4.45 KBmile23
#4 2897889-review.patch2.3 KBchaitanya17
#2 2897889-patch.patch2.31 KBchaitanya17

Comments

chaitanya17 created an issue. See original summary.

chaitanya17’s picture

chaitanya17 commented
Assigned: chaitanya17 » Unassigned
Status: Active » Needs review
StatusFileSize
new 2897889-patch.patch2.31 KB

Please check attached working patch.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

Nice catch. Good to get rid of deprecated code.

Just a few things:

  1. +++ b/config_entity_example/src/Form/RobotFormBase.php
@@ -21,9 +21,9 @@
+   * @var \Drupal\Core\Entity\EntityTypeManager
...
+  protected $entityTypeManager;

@@ -33,11 +33,11 @@
+   * @param \Drupal\Core\Entity\EntityTypeManager $entity_type_manager

@@ -126,7 +126,7 @@
+    $query = $this->entityTypeManager->getStorage('robot')->getQuery();

    What we end up doing here is getting the entity type storage for 'robot.'

    So we shouldn't get the entity type manager service, we should get the storage object.

    Also, when we do that, we want to deal with interfaces instead of concrete class types. If you look at getStorage(), it returns \Drupal\Core\Entity\EntityStorageInterface which is how we want to type hint our local variable, and the argument to __construct(). 

  2. +++ b/config_entity_example/src/Form/RobotFormBase.php
@@ -126,7 +126,7 @@
   public function exists($entity_id, array $element, FormStateInterface $form_state) {

    I looked and I couldn't find any tests of whether exists() is meaningful in our form.

    We need two more tests, inside of testConfigEntityExample():

    1) Try to re-submit Robby robot with a duplicate machine name. That should result in an error message based on exists(). (If the form is set up incorrectly, we'll get an exception complaining that it's a duplicate ID.)

    2) Try to submit a robot with the machine name 'custom' so we can make sure our form API regex catches it.

chaitanya17’s picture

chaitanya17 commented
StatusFileSize
new 2897889-review.patch2.3 KB

Hi @Mile23,

Thanks for reviewing patch. I have implemented points 1. Please check attached patch.

For Point 2. 

$robby_label = 'Custom';
    $this->drupalPostForm(
      NULL,
      array(
        'label' => $robby_label,
        'id' => $robby_machine_name,
        'floopy' => TRUE,
      ),
      t('Create Custom')
    );

In function testConfigEntityExample

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

In #3.2 I'm talking about this:

'id' => $robby_machine_name,

That's the machine name. Our exists() method complains if the machine name is the word 'custom'. We don't have a test for it, though.

Also, an interdiff makes it much easier to see what changes you made since the last patch. https://www.drupal.org/documentation/git/interdiff

Thanks.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new 2897889_6.patch4.45 KB
new interdiff.txt3.19 KB

Fixed the failing test and added the tests mentioned in #3.2.

This fixes some of the technical debt we see when we test against core 8.6.x in #2952984-19: Add example drupalci.yml file to un-suppress deprecations

  • Mile23 committed 137ae49 on 8.x-1.x authored by chaitanya17 
    Issue #2897889 by chaitanya17, Mile23: method/Class QueryFactory is...
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Fixed

Committed and pushed. Thanks!

Status: Fixed » Closed (fixed)

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