Problem/Motivation

  1. We repeat the string/docblock The interface or class that this Normalizer supports. a gazillion times.
  2. We also have lots of
    $supportedInterfaceOrClass = "Drupal\something\Something";

    instead of

    use Drupal\something\Something;
…
$supportedInterfaceOrClass = Something::class;

    which is more brittle during refactoring, and has no/worse IDE support.

Proposed resolution

For all normalizers:

  1. Use @inheritdoc
  2. Convert strings to Something::class

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#15 2926855-15.patch11.2 KBvacho
#7 2926855-7.patch12.8 KBWim Leers
#7 interdiff.txt1.62 KBWim Leers
#4 2926855-4.patch11.81 KBWim Leers
#4 interdiff.txt619 bytesWim Leers
#3 2926855-3.patch12.05 KBWim Leers
#3 interdiff.txt947 bytesWim Leers
#2 2926855-2.patch12.05 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
2926855-2.patch12.05 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt947 bytes
2926855-3.patch12.05 KB 
+++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
@@ -4,6 +4,7 @@
+use Drupal\field\Plugin\migrate\source\d6\Field;

Eh this is definitely wrong.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt619 bytes
2926855-4.patch11.81 KB

And I still got it wrong. PHPStorm--

The last submitted patch, 3: 2926855-3.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Needs work

Great clean up issue!

Missed a couple
\Drupal\serialization\Normalizer\ContentEntityNormalizer use an array and class name string
\Drupal\serialization\Normalizer\EntityNormalizer uses an array

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.62 KB
2926855-7.patch12.8 KB

Well-spotted!

The last submitted patch, 2: 2926855-2.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 7: 2926855-7.patch, failed testing. View results

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented

#7 fail is a testbot issue. Investigating

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

This patch doesn't apply anymore, and I don't think the testbot fluke in #7 is a problem.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Needs reroll, +Novice, +php-novice
vacho’s picture

vacho CreditAttribution: vacho at Skilld commented
Issue tags: -Needs reroll
FileSize
2926855-15.patch11.2 KB

Patch rerolled, solving merge conflicts.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Looks great, thank you! 👌 🙏

The last submitted patch, 7: 2926855-7.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 3302a0a and pushed to 8.7.x. Thanks!

Missed crediting tedbow on the commit, but adding review credit here...

  • catch committed 3302a0a on 8.7.x 
    Issue #2926855 by Wim Leers, vacho: Normalize  docblock + value of all...

Status: Fixed » Closed (fixed)

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