Problem/Motivation

As title

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jungle created an issue. See original summary.

jungle’s picture

First iteration

jungle’s picture

Status: Active » Needs review
Issue tags: +Global2020

For reviewer:

  1. Run cd core && yarn && yarn spellcheck:core to make sure spelling check passes.
  2. Or visit the parent issue #3122088: [Meta] Remove spelling errors from dictionary.txt and fix them/sibling issues for more/for background/for reference.
ultrabob’s picture

I'll work on reviewing this.

ultrabob’s picture

Status: Needs review » Reviewed & tested by the community
  • I remade the dictionary (I saw a few more additional changes show up: removing 'upserting' and changing '<C3><BC>bersetzung' to '<C3><9C>bersetzung' for example. That seem unrelated to this patch)
  • Then I ran spellcheck and got no errors.

    /var/www/html/core$ yarn run spellcheck:core
    yarn run v1.22.4
    $ cspell "**/*" "../composer/**/*" "../composer.json"
    CSpell: Files checked: 14630, Issues found: 0 in 0 files
    Done in 451.19s.
  • Then just to be sure spellcheck itself was working properly I added the patchfile into the core directory since I know it has spelling errors included. Running the spellcheck again flagged those errors.

I think this one is good to go!

jungle’s picture

Thanks @ultrabob for reviewing.

Self-review:

+++ b/core/modules/aggregator/tests/src/Functional/FeedParserTest.php
@@ -64,16 +64,16 @@ public function testAtomSample() {
+    $ids = \Drupal::entityQuery('aggregator_item')->condition('link', 'http://example.org/2003/12/13/atom03')->execute();
+    $item = Item::load(array_values($ids)[0]);

$iids was refactored to $ids, but maybe changing it to $item_ids is better. But $ids is ok probably. So Stay RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php
@@ -197,10 +197,10 @@ public function updateFeedItems(FeedInterface $feed, $expected_count = NULL) {
-    $iids = \Drupal::entityQuery('aggregator_item')->condition('fid', $feed->id())->execute();
+    $ids = \Drupal::entityQuery('aggregator_item')->condition('fid', $feed->id())->execute();
     $feed->items = [];
-    foreach ($iids as $iid) {
-      $feed->items[] = $iid;
+    foreach ($ids as $id) {
+      $feed->items[] = $id;
     }

+++ b/core/modules/aggregator/tests/src/Functional/FeedParserTest.php
@@ -64,16 +64,16 @@ public function testAtomSample() {
-    $iids = \Drupal::entityQuery('aggregator_item')->condition('link', 'http://example.org/2003/12/13/atom03')->execute();
-    $item = Item::load(array_values($iids)[0]);
+    $ids = \Drupal::entityQuery('aggregator_item')->condition('link', 'http://example.org/2003/12/13/atom03')->execute();
+    $item = Item::load(array_values($ids)[0]);
...
-    $iids = \Drupal::entityQuery('aggregator_item')->condition('link', 'http://example.org/2003/12/14/atom03')->execute();
-    $item = Item::load(array_values($iids)[0]);
+    $ids = \Drupal::entityQuery('aggregator_item')->condition('link', 'http://example.org/2003/12/14/atom03')->execute();
+    $item = Item::load(array_values($ids)[0]);

+++ b/core/modules/aggregator/tests/src/Functional/UpdateFeedItemTest.php
@@ -54,8 +54,8 @@ public function testUpdateFeedItem() {
-    $iids = \Drupal::entityQuery('aggregator_item')->condition('fid', $feed->id())->execute();
-    $before = Item::load(array_values($iids)[0])->getPostedTime();
+    $ids = \Drupal::entityQuery('aggregator_item')->condition('fid', $feed->id())->execute();
+    $before = Item::load(array_values($ids)[0])->getPostedTime();

@@ -67,7 +67,7 @@ public function testUpdateFeedItem() {
-    $after = Item::load(array_values($iids)[0])->getPostedTime();
+    $after = Item::load(array_values($ids)[0])->getPostedTime();

Here iids means item IDs. I think changing this to $item_ids is better because if we also have to deal with fids - ie. feed IDs then things are less confusing.

ravi.shankar’s picture

Assigned: Unassigned » ravi.shankar
ravi.shankar’s picture

Assigned: ravi.shankar » Unassigned
Status: Needs work » Needs review
FileSize
10.02 KB
3.92 KB

Here I have addressed comment #7, please review.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php
@@ -197,10 +197,10 @@ public function updateFeedItems(FeedInterface $feed, $expected_count = NULL) {
-    $iids = \Drupal::entityQuery('aggregator_item')->condition('fid', $feed->id())->execute();
+    $item_ids = \Drupal::entityQuery('aggregator_item')->condition('fid', $feed->id())->execute();
     $feed->items = [];
-    foreach ($iids as $iid) {
-      $feed->items[] = $iid;
+    foreach ($item_ids as $item_id) {
+      $feed->items[] = $item_id;
     }

Looking at it this is a bit odd. It could be

$item_ids = \Drupal::entityQuery('aggregator_item')->condition('fid', $feed->id())->execute();
$feed->items = array_values($item_ids);

for quite a bit less code. Given we're changing every line I think this is okay.

For what it is worth the items property on the Feed entity needs work. It's dynamically declared, only has meaning whilst a feed is being updated. It's all a bit odd.

jungle’s picture

Status: Needs work » Needs review
FileSize
10.04 KB
823 bytes

@alexpott, thanks for the review!

for quite a bit less code. Given we're changing every line I think this is okay.

Addressing.

For what it is worth the items property on the Feed entity needs work. It's dynamically declared, only has meaning whilst a feed is being updated. It's all a bit odd.

Out of scope there to rewrite the whole. Leaving it to a separate issue if necessary.

alexpott’s picture

Leaving it to a separate issue if necessary.

Yeah I was not suggesting to tackle that here - definitely a separate issue.

samiullah’s picture

We installed the fresh version of drupal 9.1
Applied the patch
Ran cd core && yarn && yarn spellcheck:core
No spelling errors were observed
test

@alexpott this can be moved to RTBC if code review part is fine

jungle’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @samiullah, let's set back to RTBC.

BTW, you do not have to take a screenshot. To copy and paste the output is enough, wrapping with the code tag.

alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 8e736dc and pushed to 9.1.x. Thanks!
Committed c689c8e and pushed to 9.0.x and cherry-picked to 8.9.x. Thanks!

As this is only test changes backported to 8.9.x for consistency.

I didn't created @samiullah since as @jungle says screenshots are not required.

  • alexpott committed 8e736dc on 9.1.x
    Issue #3160020 by jungle, ravi.shankar, alexpott, ultrabob: Fix typos "...

  • alexpott committed c689c8e on 9.0.x
    Issue #3160020 by jungle, ravi.shankar, alexpott, ultrabob: Fix typos "...

  • alexpott committed d4a43eb on 8.9.x
    Issue #3160020 by jungle, ravi.shankar, alexpott, ultrabob: Fix typos "...

Status: Fixed » Closed (fixed)

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