Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up of #1937600: Determine what services to register in the new Drupal class
Replace all calls to typed_data() with Drupal::typedData() and then remove the typed_data() function.
Comment | File | Size | Author |
---|---|---|---|
#17 | fix-to-use-typeddata-1957158-17.patch | 12.04 KB | JacobSanford |
#13 | 1957158-fix-to-use-typedData.patch | 12.51 KB | abghosh82 |
#10 | 1957158-fix-to-use-typedData.patch | 12.44 KB | abghosh82 |
#8 | 1957158-fix-to-use-typedData.patch | 14.03 KB | abghosh82 |
#5 | replace_typed_data_with_DrupaltypedData-issue#1957158-comment#7265174.patch | 14.03 KB | abghosh82 |
Comments
Comment #1
BerdirInstructions:
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".
Comment #2
xjmPostponing on the main issue.
Comment #3
BerdirComment #4
abghosh82 CreditAttribution: abghosh82 commentedComment #5
abghosh82 CreditAttribution: abghosh82 commentedPosting the patch file.
Comment #7
BerdirYou 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 :)
Comment #8
abghosh82 CreditAttribution: abghosh82 commentedThanks for the suggestion, I am new to contribution, was not sure about the patch file name.
Comment #9
BerdirPer our coding standards, we don't add use's for classes without a namespace but just reference them inline as \Drupal.
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.
Comment #10
abghosh82 CreditAttribution: abghosh82 commentedImplemented the code review comments.
Comment #11
BerdirThanks, I think this looks good now. We'll have to try and inject these calls, but let's get this conversion in first.
Comment #12
alexpottWe already have the container injected into tests... lets us it :)
Should be
$this->container->get('typed_data')->create($definition);
Should be
$this->container->get('typed_data')->create($entity_definition, $entity);
Should be
$this->container->get('typed_data')->create($entity_definition, $entity);
Should be
$this->container->get('typed_data')->create($entity_definition, $node);
Should be
$this->container->get('typed_data');
Not sure why this is not
$this->typedData->create($definition, $value, $name);
given that the snippet above this one is from the same test.Comment #13
abghosh82 CreditAttribution: abghosh82 commentedThanks for pointing this out, I was really not sure of the injected containers. Implemented the suggestions.
Comment #14
BerdirNot 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.
Comment #15
abghosh82 CreditAttribution: abghosh82 commentedComment #16
abghosh82 CreditAttribution: abghosh82 commented@alexpott Please review the changes.
Comment #17
JacobSanfordReroll!
Comment #18
BerdirLooks good, back to RTBC.
Comment #19
alexpottCommitted 521e1e5 and pushed to 8.x. Thanks!