In #2137317: Some questions about aggregator in d8 , dasjo said:

also note that aggregator fails when a feed item contains an empty description:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[HY000]: General error: 1364 Field 'description' doesn't have a default value: INSERT INTO {aggregator_item} (iid, description, fid, title, langcode, link, author, timestamp, guid) VALUES (default, default, :db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array ( [:db_insert_placeholder_0] => 18 [:db_insert_placeholder_1] => ... [:db_insert_placeholder_2] => und [:db_insert_placeholder_3] => ... [:db_insert_placeholder_4] => ... [:db_insert_placeholder_5] => 1338129317 [:db_insert_placeholder_6] => ... at ... ) in Drupal\Core\Entity\FieldableDatabaseStorageController->save() (line 693 of /../core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php).

ParisLiaskos said:

Ugh, that bug needs an issue for sure.

I ran into this issue trying to add my personal delicious feed to Aggregator. Steps to reproduce, assuming you have a feed that has items with no description:

  1. Add the feed as a new feed at admin/config/services/aggregator
  2. Click "Update items" under "Operations"
CommentFileSizeAuthor
#11 core-aggregator-empty-item-description-2187233-11.patch3.85 KBeelkeblok
#7 core-aggregator-empty-item-description-2187233-6.patch2.75 KBeelkeblok
#1 core-aggregator-empty-item-description-2187233.patch1 KBeelkeblok

Comments

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
StatusFileSize
new core-aggregator-empty-item-description-2187233.patch1 KB

I attached a patch that solves the problem. The NULL value is explicitly set in the Zend\Feed\Reader\Entry\Rss in case of an empty value. I was not 100% sure where would be the appropriate location to test for the NULL value, but since author and timestamp had similar mechanisms, I thought this should be OK.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Status: Active » Needs review
eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented

One question remains, btw; should we also allow for the other fields to be NULL (i.e. title, guid and link)?

ParisLiakos’s picture

ParisLiakos commented
Version: 8.0-alpha8 » 8.x-dev
Status: Needs review » Needs work
Issue tags: +Needs tests

thanks for the patch!
But i think that this check should happen in the processor instead before calling setDescription to the item entity.

I think you are right..i think we should also check for the link and also maybe the guid for being null before setting them.
Also we should add a test for this

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented

Not the title? Obviously, especially when all of them would be empty something is up, but we shouldn't break on a database update then, I guess.

Either way, thanks for your input. This is my first core patch, so tests wasn't something on my radar, but I realized later that would probably be required too. I'll have a crack at that too, this seems like a simple enough issue to serve as an "introduction to core patches" study subject :)

ParisLiakos’s picture

ParisLiakos commented

Oh, welcome then, and thanks for your contribution!

About the title:

      if (empty($item['title'])) {
        continue;
      }

This is in the processor^

For tests, adding an entry in
core/modules/aggregator/tests/modules/aggregator_test/aggregator_test_rss091.xml
with no description should be enough.

Btw i just added an entry like this:

    <item>
      <title>test</title>
      <link></link>
      <author></author>
      <description></description>
    </item>

It only complained about the description. no problem about the empty link or empty author

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new core-aggregator-empty-item-description-2187233-6.patch2.75 KB

OK, here's another stab, with additions to the test feed to trigger the problem. I added a few more items to the feed, basically one item with all fields empty, and then one for each case where one field is empty, assuming that would cover any possible combination of empty fields. This to catch any problems like this down the line. As you found, it was only needed to take special action for the empty description, at this point.

Edit: Whoops, I misnumbered the patch.

Status: Needs review » Needs work

The last submitted patch, 7: core-aggregator-empty-item-description-2187233-6.patch, failed testing.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented

Ah, that's a shame, the patch trips up the test "Remove feed item functionality" with the message "Total items in feed equal to the total items in database (4 != 7)". I did re-run all aggregator tests, not sure why I didn't pick this up. Probably at least the items with a missing title should not be in the feed with "valid" items because of the code in #6. That leaves one item unaccounted for. Indeed, needs work.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Assigned: Unassigned » eelkeblok
eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Status: Needs work » Needs review
StatusFileSize
new core-aggregator-empty-item-description-2187233-11.patch3.85 KB

It was a simple fix, really, the failing test was asserting on the number of items imported from the feed, which obviously changed with the extra test cases.

Status: Needs review » Needs work

The last submitted patch, 11: core-aggregator-empty-item-description-2187233-11.patch, failed testing.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review

11: core-aggregator-empty-item-description-2187233-11.patch queued for re-testing.

ParisLiakos’s picture

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

Exactly:)
Thank you!!

alexpott’s picture

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

Nice find. Committed 134cfbe and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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