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

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Active » Needs review
StatusFileSize
new feed_id_should_be-2602662-2.patch1.93 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
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 🇪🇺🌍
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 Pittsfield, MA, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 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
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 Pittsfield, MA, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 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
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
commented
Status: Needs work » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
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 Pittsfield, MA, USA
commented
StatusFileSize
new 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 Pittsfield, MA, USA
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 Pittsfield, MA, USA
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 🇪🇺🌍
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 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 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.