We usually have a UUID field on each entity type using the 'uuid' field type, but aggregator items have a GUID 'string' field instead. We should convert and rename it to be in line with the rest of core.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Postponed

Forgot to say that I'd prefer this to wait for #2112239: Convert base field and property definitions.

Berdir’s picture

It's called GUID because that's what it's called in RSS, it doesn't have to be a UUID nor is it generated by us like our UUID's usually are.

If anything, we should add an additional UUID, if you want to synchronize something internally, but that doesn't make much sense to me?

amateescu’s picture

Status: Postponed » Closed (works as designed)

I see :) Ok, let's close this one.

amateescu’s picture

Status: Closed (works as designed) » Needs review
FileSize
741 bytes

Err.. I meant this.

Berdir’s picture

Title: Weird GUID field on the 'aggregator_item' entity » Remove todo about GUID field on the 'aggregator_item' entity
Status: Needs review » Reviewed & tested by the community

Works for me, updating title accordingly

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

How about actually documenting this here so we're not tempted to change this is the future?

tstoeckler’s picture

Also Item::uuid() should override ContentEntityBase::uuid() to always return NULL. On the other hand the latter should really be checking for the UUID key, similar to #2182239: Improve ContentEntityBase::id() for better DX so might just want to leave it as is for now.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

Version: 8.1.x-dev » 8.2.x-dev
amateescu’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
863 bytes

Changed the @todo into a better description of the GUID field.

Also Item::uuid() should override ContentEntityBase::uuid() to always return NULL.

I would prefer to not do this, just in case someone needs to add a 'real' UUID field to feed items for deployment purposes :)

Status: Needs review » Needs work

The last submitted patch, 10: 2149851-10.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
968 bytes

+1 for #10, because if we use GUIDs as if they are UUIDs, we're very likely to run into two different Item entities having the same GUID, which of course defeats the purpose!

However, that means that we chose not to add a uuid field to the Item content entity type for no reason at all: we should have done that after all! Right?


Attached is a straight reroll of #10.

Wim Leers’s picture

Title: Remove todo about GUID field on the 'aggregator_item' entity » Remove todo about GUID field on the 'aggregator_item' entity and add UUID field
Issue tags: +Needs update path, +Needs update path tests, +Contributed project blocker
FileSize
461 bytes
1.09 KB

This then adds a uuid entity key base field. Thanks to \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions, this means Item content entities will automatically get a UUID field. However, this of course needs an update path for existing Item content entities.

This is the only content entity type that does not have UUIDs. It prevents e.g. the https://www.drupal.org/project/jsonapi contrib module from being able to work with Item content entities.

Let's see what fails. (But we'll still need an update path anyway.)

Status: Needs review » Needs work

The last submitted patch, 15: 2149851-15.patch, failed testing.

Wim Leers’s picture

I think the update path test coverage needs to do two things:

  1. test that existing Item entities do not have a UUID before the update, but do so afterwards. Example: system_update_8400()
  2. test that the Item entity type does not have a UUID key before the update, but does so afterwards. Example: \Drupal\node\Tests\Update\NodeUpdateTest.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

JacobSanford’s picture

FileSize
1013 bytes
1.47 KB

@Wim Leers's patch in #15 no longer applied to 8.9.x. A reroll with no further modifications is attached - items mentioned in #17 still need to be addressed.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Project: Drupal core » Aggregator
Version: 9.4.x-dev » 1.x-dev
Component: aggregator.module » Code

The aggregator module has been removed from Core in 10.0.x-dev and now lives on as a contrib module. Issues in the Core queue about the aggregator module, like this one, have been moved to the contrib module queue.

larowlan’s picture

Issue tags: +Needs reroll

Needs a reroll for contrib

Arti Anil Pattewar’s picture

FileSize
6.88 KB

Rerolled patch #23 for contrib Aggregator 1.0 on D9.4.x.

Arti Anil Pattewar’s picture

Status: Needs work » Needs review
FileSize
8.03 KB
dcam’s picture

Issue tags: -Needs reroll
FileSize
916 bytes

Rerolled #23.

dcam’s picture

Version: 1.x-dev » 2.x-dev
dcam’s picture

Category: Bug report » Task
FileSize
1.44 KB

Updated the test array to try and fix the errors in #32.

dcam’s picture

Status: Needs review » Needs work

NW for the update paths. I started work on them, but haven't finished.

larowlan’s picture

Looks good, just needs update path as you mentioned

dcam’s picture

Status: Needs work » Needs review
Issue tags: -Needs update path, -Needs update path tests
FileSize
5.7 KB
5.7 KB

I added the update path and test.

dcam’s picture

+++ b/tests/src/Functional/Update/AggregatorUpdateAddItemUuid.php
@@ -0,0 +1,34 @@
+  /**
+   * Ensure all items in the aggregator_feeds queue are deleted.
+   */
+  public function testUpdateHookN(): void {

I forgot to update this docblock when I copied it from another test I'd written. But I'm not going to bother changing it right now. We'll probably have to update this class's docblock to change the number of the update function it covers anyway. It can be fixed then.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Good to go, will need a reroll when one of the other update hook issues goes in, so fix the docblock at the same time

andypost’s picture

+++ b/aggregator.install
@@ -11,3 +11,15 @@
 function aggregator_update_last_removed() {
   return 8501;
...
+function aggregator_update_8601() {

both places will need to check and fix on commit

dcam’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.83 KB

Rerolled #37

  • dcam committed 839c5303 on 2.x
    Issue #2149851 by dcam, Wim Leers, amateescu, Arti Anil Pattewar,...

  • dcam committed 4cb9e424 on 1.x
    Issue #2149851 by dcam, Wim Leers, amateescu, Arti Anil Pattewar,...
dcam’s picture

Status: Needs review » Fixed

Thank you to everyone who worked on this!

Status: Fixed » Closed (fixed)

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