Problem/Motivation

The aggregator_item entity does not declare the fid (feed ID) base field to be required, but saving an aggregator_item without a fid will fatal error in getCacheTagsToInvalidate() trying to reference the non-existent feed ID. This is a problem for me in #2590993: Create stub entities with proper default values - that issue will populate required fields with appropriate values when creating migration stubs, but needs to know that a field is required to do that.

Proposed resolution

A simple setRequired(TRUE) on the base field. This is verified to work with my stubbing patch.

Remaining tasks

Submit a patch with a test.

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#13 feed_id_should_be-2602662-13-test-only.patch1.21 KBmikeryan
#7 feed_id_should_be-2602662-7.patch2.44 KBmikeryan
#5 feed_id_should_be-2602662-5.patch1.23 KBmikeryan
#2 feed_id_should_be-2602662-2.patch1.93 KBmikeryan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Active » Needs review
FileSize
feed_id_should_be-2602662-2.patch1.93 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +rc target triage 
+++ b/core/modules/aggregator/src/Tests/ItemWithoutFeedTest.php
@@ -0,0 +1,47 @@
+  public function testEntityCreation() {
+    $entity = Item::create([
+                   'title' => t('Llama 2'),
+                   'path' => 'https://groups.drupal.org/',
+                 ]);
+    $violations = $entity->validate();
+    $this->assertCount(1, $violations);
+    // A save() at this point would fatal error.

The indentation around Item::create() is strange, and it might be helpful to assert the constraint violation message (or something).

Other than that, looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work

We need an empty hook_update_N to force a cache clear.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs work » Needs review
FileSize
feed_id_should_be-2602662-5.patch1.23 KB

Indentation fixed.

Removed the comment at the end of the test, which I think is only a distraction... That reflects the original symptom, but the fix is being tested here.

Added an update function.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work

Hmmm, the test seems to have vanished from this patch :(

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs work » Needs review
FileSize
feed_id_should_be-2602662-7.patch2.44 KB

Create feature branch
Apply patch that creates new file
Commit changes to feature branch <- easy to forget this step
git diff 8.0.x

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Ace. Pre-RTBC assuming DrupalCI passes.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: feed_id_should_be-2602662-7.patch, failed testing.

The last submitted patch, 7: feed_id_should_be-2602662-7.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review
Issue tags: -rc target triage +rc target, +migrate

Discussed with @xjm, @alexpott and @effulgentsia.

We agreed this should be an RC target. It would be nice to see a test only patch exposing the failure.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
FileSize
feed_id_should_be-2602662-13-test-only.patch1.21 KB

Failure exposed!

Status: Needs review » Needs work

The last submitted patch, 13: feed_id_should_be-2602662-13-test-only.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs work » Reviewed & tested by the community

As expected.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: feed_id_should_be-2602662-13-test-only.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs work » Reviewed & tested by the community

Enough with the fail test already...

alexpott’s picture

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

Committed 22716ab and pushed to 8.0.x. Thanks!

+++ b/core/modules/aggregator/aggregator.install
@@ -21,3 +21,19 @@ function aggregator_requirements($phase) {
+ * @addtogroup updates-8.0.0-beta
...
+ * @} End of "addtogroup updates-8.0.0-beta".

changed to rc on commit.

  • alexpott committed 22716ab on 8.0.x 
    Issue #2602662 by mikeryan, phenaproxima: Feed ID should be required...

Status: Fixed » Closed (fixed)

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

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 
+++ b/core/modules/aggregator/aggregator.install
@@ -21,3 +21,19 @@ function aggregator_requirements($phase) {
+function aggregator_update_8001() {
+  // Feed ID base field is now required.
+}

Note that this is not correct. Adding an empty update function does not update the "last installed" field storage definition automatically.

Since we shouldn't touch existing update functions, this is being fixed in #2346019-25: Handle initial values when creating a new field storage definition, see the system_update_8300() function from that patch.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

On second thought, I opened a separate followup issue to fix this: #2840595: The 'Source feed' field of aggregator items has to be updated and marked as required.