This is side issue from https://www.drupal.org/node/1807160

looking at the output from

phpcs --standard=Drupal ./core/modules/aggregator/

There are some

indentation issues
missing public/private from functions.
new line messages between things like @group and @see and between @param and @return

CommentFileSizeAuthor
#7 interdiff-5-7.txt1.31 KBmartin107
#7 aggregator-2424745-7.patch29.5 KBmartin107
#5 interdiff-3.5.txt8.6 KBmartin107
#5 aggregator-2424745-5.patch30.32 KBmartin107
#3 aggregator-2424745-3.patch24.89 KBmartin107
#1 aggregator-2424745-1.patch24.86 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Active » Postponed
FileSize
aggregator-2424745-1.patch24.86 KB

Just supplying an initial clean up.

I have more changes in mind, but postponing because I want to see the parent get home first.

For the visibility issue, whether to add public or protected etc to a function... I added public, except in one case where protected was appropriate.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Component: documentation » aggregator.module

These are not documentation changes, so changing component.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Postponed » Needs review
FileSize
aggregator-2424745-3.patch24.89 KB

Needed reroll. No conflicts just auto merging.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented 
+++ b/core/modules/aggregator/src/Tests/UpdateFeedTest.php
@@ -16,14 +16,14 @@ class UpdateFeedTest extends AggregatorTestBase {
-      $edit['refresh'] =  1800; // Change refresh value.
+      $edit['refresh'] = 1800; // Change refresh value.

Shouldnt the comment go to a separate line?

martin107’s picture

martin107 CreditAttribution: martin107 commented
FileSize
aggregator-2424745-5.patch30.32 KB
interdiff-3.5.txt8.6 KB

Thanks, fix.

I have done more work on this..

Added a few more @inheritdocs
Added some trivial doc comments,
Fix more PUBLIC functions statements

There are some variables that need converting from underscore format to camel case but the size of this patch is becoming too large
and I want this patch to be a sequence of obvious gotcha, that are quick to review.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

thank you, for working on this :)

  1. +++ b/core/modules/aggregator/src/Plugin/QueueWorker/AggregatorRefresh.php
@@ -11,6 +11,8 @@
+ * Ensures the feed is updated.

    Maybe "Updates a feed's items." is more accurate description 

  2. +++ b/core/modules/aggregator/src/Plugin/aggregator/processor/DefaultProcessor.php
@@ -192,8 +192,8 @@ public function process(FeedInterface $feed) {
       // @todo: The default entity view builder always returns an empty
-      //   array, which is ignored in aggregator_save_item() currently. Should
-      //   probably be fixed.
+      // array, which is ignored in aggregator_save_item() currently. Should
+      // probably be fixed.

    i think this is correct according to
    https://www.drupal.org/node/1354#todo

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: martin107 » Unassigned
FileSize
aggregator-2424745-7.patch29.5 KB
interdiff-5-7.txt1.31 KB

1) Yes text is better.

2) I agree - backed out changes.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

thank you!

The last submitted patch, 1: aggregator-2424745-1.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Coding standards changes are not blocked in beta - and this one is not disruptive as aggregator module is not changing a lot at the moment. Committed 38ea2f1 and pushed to 8.0.x. Thanks!

  • alexpott committed 38ea2f1 on 8.0.x 
    Issue #2424745 by martin107: aggregator module document tidy

Status: Fixed » Closed (fixed)

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