Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Title: Replace calls of typed_data() with Drupal::typedData() » Replace typed_data() with Drupal::typedData()
Issue tags: +Novice

Instructions:

Search & replace all calls to typed_data() with the mentioned method. Note that calls within namespaced classes (have a namespace line and are in the lib/ folder) needs to be "\Drupal".

xjm’s picture

Status: Active » Postponed

Postponing on the main issue.

Berdir’s picture

Status: Postponed » Active
abghosh82’s picture

Assigned: Unassigned » abghosh82
abghosh82’s picture

Status: Active » Needs review
FileSize
14.03 KB

Posting the patch file.

Status: Needs review » Needs work
Berdir’s picture

You need to re-upload that patch with special charaters or whitespaces, the testbot doesn't like that. Also you probably want to use a name that's a bit shorter :)

abghosh82’s picture

Status: Needs work » Needs review
FileSize
14.03 KB

Thanks for the suggestion, I am new to contribution, was not sure about the patch file name.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -7,6 +7,7 @@
+use Drupal;

@@ -170,7 +171,7 @@ protected function getTranslatedField($property_name, $langcode) {
+        $this->fields[$property_name][$langcode] = Drupal::typedData()->getPropertyInstance($this, $property_name, $value);

@@ -316,7 +317,7 @@ public function getTranslation($langcode, $strict = TRUE) {
+    $translation = Drupal::typedData()->create($translation_definition, $fields);

Per our coding standards, we don't add use's for classes without a namespace but just reference them inline as \Drupal.

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.phpundefined
@@ -130,7 +130,7 @@ public function createInstance($plugin_id, array $configuration, $name = NULL, $
-   * @see typed_data()
+   * @see Drupal::typedData()

I guess the @see should also be \Drupal::typedData()

This is a special case as it seems we only have cases that should be injected and we will hopefully get there but for now, let's do this simple conversion. We could still get usages of this in procedural code later on.

abghosh82’s picture

Status: Needs work » Needs review
FileSize
12.44 KB

Implemented the code review comments.

Berdir’s picture

Assigned: abghosh82 » Unassigned
Status: Needs review » Reviewed & tested by the community

Thanks, I think this looks good now. We'll have to try and inject these calls, but let's get this conversion in first.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

We already have the container injected into tests... lets us it :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.phpundefined
@@ -357,7 +357,7 @@ protected function assertIntrospection($entity_type) {
+    $wrapped_entity = \Drupal::typedData()->create($definition);

Should be $this->container->get('typed_data')->create($definition);

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.phpundefined
@@ -477,7 +477,7 @@ protected function assertDataStructureInterfaces($entity_type) {
+    $wrapped_entity = \Drupal::typedData()->create($entity_definition, $entity);

Should be $this->container->get('typed_data')->create($entity_definition, $entity);

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.phpundefined
@@ -536,7 +536,7 @@ public function testEntityConstraintValidation() {
+    $wrapped_entity = \Drupal::typedData()->create($entity_definition, $entity);

Should be $this->container->get('typed_data')->create($entity_definition, $entity);

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.phpundefined
@@ -564,7 +564,7 @@ public function testEntityConstraintValidation() {
+    $wrapped_entity = \Drupal::typedData()->create($entity_definition, $node);

Should be $this->container->get('typed_data')->create($entity_definition, $node);

+++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataTest.phpundefined
@@ -42,7 +42,7 @@ public function setUp() {
+    $this->typedData = \Drupal::typedData();

Should be $this->container->get('typed_data');

+++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataTest.phpundefined
@@ -54,7 +54,7 @@ protected function createTypedData($definition, $value = NULL, $name = NULL) {
+    $data = \Drupal::typedData()->create($definition, $value, $name);

Not sure why this is not $this->typedData->create($definition, $value, $name); given that the snippet above this one is from the same test.

abghosh82’s picture

Status: Needs work » Needs review
FileSize
12.51 KB

Thanks for pointing this out, I was really not sure of the injected containers. Implemented the suggestions.

Berdir’s picture

Not too sure about using $this->container in tests. I already used \Drupal::something() in various other patches, e.g. Guzzle conversions. With $this->container, we don't get autocomplete for methods and there are cases where $this->container and Drupal::getContainer() aren't the same object anymore, due to rebuilds or so. Might not affect this but was a problem for config/state cache clears in other issues.

abghosh82’s picture

Assigned: Unassigned » abghosh82
abghosh82’s picture

@alexpott Please review the changes.

JacobSanford’s picture

Reroll!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 521e1e5 and pushed to 8.x. Thanks!

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