Problem/Motivation

Adhere to the automated coding standards.

CommentFileSizeAuthor
#18 combined-2801919-18.patch53.7 KBmartin107
#18 nudges-2801919-18-DO_NOT_COMMIT.patch.txt1.93 KBmartin107
#12 2801919-12.patch54.07 KBmartin107
#8 2801919--minor-cs-fixes--8.patch47.64 KBe0ipso
#5 2801919--minor-cs-fixes--5.patch47.63 KBe0ipso
#2 2801919--minor-cs-fixes--2.patch47.47 KBe0ipso
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

e0ipso created an issue. See original summary.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Active » Needs review
FileSize
2801919--minor-cs-fixes--2.patch47.47 KB

Merging if green.

Status: Needs review » Needs work

The last submitted patch, 2: 2801919--minor-cs-fixes--2.patch, failed testing.

The last submitted patch, 2: 2801919--minor-cs-fixes--2.patch, failed testing.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Needs work » Needs review
FileSize
2801919--minor-cs-fixes--5.patch47.63 KB

Moar fixes.

Status: Needs review » Needs work

The last submitted patch, 5: 2801919--minor-cs-fixes--5.patch, failed testing.

The last submitted patch, 5: 2801919--minor-cs-fixes--5.patch, failed testing.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Needs work » Needs review
FileSize
2801919--minor-cs-fixes--8.patch47.64 KB

:weary:

Status: Needs review » Needs work

The last submitted patch, 8: 2801919--minor-cs-fixes--8.patch, failed testing.

The last submitted patch, 8: 2801919--minor-cs-fixes--8.patch, failed testing.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

Bump.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Assigned: Unassigned » martin107
Status: Needs work » Needs review
FileSize
2801919-12.patch54.07 KB

running phpcbf on todays code base, here is the result.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Assigned: martin107 » Unassigned

There are lots of cases of 

+
+  /**
+   *
+   */
   protected function getExpectedBcUnauthorizedAccessMessage($method) {}

But this patch is already large and difficult to review.
I suggest we populate the comment blocks in another issue.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Needs review » Needs work

This looks good. However, I'd rather get rid of the empty comment blocks in this issue. I believe that if we don't do it now we'll get stuck with those for a while.

Ideally we could disable the need for method comments for testing methods. Do you know if that is possible with phpcs?

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Needs work » Postponed
Related issues: +#2870132: RestJsonApiUnsupported: missing inheritdoc, +#2870130: NullFieldNormalizerValue missing inheritdoc , +#2870131: Implement the interface correctly on NullFieldNormalizerValue

Do you know if that is possible with phpcs?

I have no idea

My preference is that is is easier mentally to fix 101 small, trivial to inspect issues rather than one monster patch.

Sorry if others find it annoying.

When I do it this way things like the object chaining issue pop out --- when normally my brain would have skipped over it.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

My preference is that is is easier mentally to fix 101 small, trivial to inspect issues rather than one monster patch.

As a module maintainer, however, I much rather to review, manage and keep in mind a single patch than 101. Otherwise it becomes an unmaintainable burden for me that never gets committed.

Besides, IMO all the work here should go out as a single commit.

Thanks for understanding!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

One thing I really like in contrib modules: Ship with thephpcs.xml.distfile your code is actually compatible with.This allows people to easily check their changes etc. When you have a phpcs.xml.dist file you can also simply use phpcbf and automate your fixes.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Postponed » Needs review
FileSize
nudges-2801919-18-DO_NOT_COMMIT.patch.txt1.93 KB
combined-2801919-18.patch53.7 KB

While I don't doubt there goodness of having a phpcs.xml.dist

I think that is a conversation for a separate issue - unless you want to blindly accept the perfectly good phpcs.xml.dist file from core.

There are two patches here

1) nudges-2801919-18-DO_NOT_COMMIT.patch.txt
Has all the changes/nudges from the other split off issues.
2) combined-2801919-18.patch
The combined result of applying the nudges and running phpcbf

I have done this so that if this issue is delayed until after Drupalcon Baltimore [ where I anticipate lots of commits.] - we can then easily reapply that patch and run phpcbf again.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented

Sorry I forgot to respond to https://www.drupal.org/node/2870130#comment-12041496

+++ b/src/Normalizer/Value/NullFieldNormalizerValue.php
@@ -11,33 +11,59 @@ class NullFieldNormalizerValue implements FieldNormalizerValueInterface {

+ /**
+ * The property type.
+ *
+ * @var mixed
+ */
protected $propertyType;

I would have expected that the property type 'attributes' or 'relationships' is a string rather than mixed.

I was influenced by the annonation for FieldNormalizerValueInterface::setPropertyType() - which implies mixed - am I being too mechanical?

  • e0ipso committed 10ffc01 on 8.x-1.x authored by martin107 
    feat(Maintainability): Fix coding standards with phpcbf (by #2801919 by...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented 
+++ b/src/Exception/EntityAccessDeniedHttpException.php
@@ -8,6 +8,9 @@ use Drupal\Core\DependencyInjection\DependencySerializationTrait;
+/**
+ *
+ */

I'll fix this on commit.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Needs review » Fixed
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

Thanks for this fix @martin107!!!

Status: Fixed » Closed (fixed)

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