Allow revisionable entities to be saved as new revisions

If I'm right, in the D7 version of Feeds, a revision is automatically created if on the content type the option "Create new revision" is turned on. Shouldn't Feeds in D8 follow that setting as well?

@MegaChriz That was my first thought when discovering this issue/patch - why we can't lean on the Revision setting from the entity bundle.

In my testing of a fairly vanilla D8 site, when importing entities that DO have the "Create new revision" checkbox checked in the bundle settings, it does create new revisions when they import into Drupal.

I guess there could be a use case here for custom entities or other things that maybe won't have that setting available on the bundle, but are still revision-able per the implemented interface.

It would be great if we had a test for this feature. I think a Kernel test would suffice.

I've tested this but it seems to set the new revision date/time to the same as the previous revision.

I've tried giving the "revision created" field a mapping to "Feed: last import" but that does as expected and uses the previous date/time the feed was run not the current time.

Cheers Dan

I tried this patch, but still the revision tables are there and growing.
The setting is unchecked in the feeds advanced section and in the content type.
I am importing from CSV to nodes and do not need revisions.

Of course I may have done something wrong, I edited the files manually and applied the changes like that.
Maybe I should have restared PHP, but I can't currently do that.

I'm intrigued that some people on this thread are able to have new revisions of nodes when a feed is run. I have "Create new revision" ticked on the content type and the feeds set to "Update existing content items" and I do not get new revisions - the current revision is simply updated. Could I be missing something obvious to make that work? This is without the patch on this ticket applied.

The old #8 patch still works ok except that it indeed sets the newly updated revision timestamp to be identical to the previous revision.

To get the new revision date saved correctly to the time of the new import we just need to add this:

$entity->setRevisionCreationTime(REQUEST_TIME);

#12: @iancawthorne that setting only affects the node UI. It sets a default value for the node form rather than setting a fixed behaviour for a node type.

#13: I am surprised that the revision creation time doesn't default to the request time. Have we uncovered a core bug here? Also have we considered the revisionUser?

#14: @bibo did you deliberately prevent the testbot running your patch? If you submit it without the .txt extension it would run.

Marking needs work for test coverage. This module is *huge* so we really need to lock in behaviours once they're decided.

The merge request opened above contains the work done at #14, made some minor code changes like

1. +++ b/src/Feeds/Processor/EntityProcessorBase.php
   @@ -160,6 +160,11 @@ abstract class EntityProcessorBase extends ProcessorBase implements EntityProces
   +        $entity->setRevisionCreationTime(REQUEST_TIME);
   

   Replaced the deprecated REQUEST_TIME with \Drupal::time()->getRequestTime().

2. +++ b/src/Feeds/Processor/EntityProcessorBase.php
   @@ -553,6 +557,7 @@ abstract class EntityProcessorBase extends ProcessorBase implements EntityProces
   +      'revision' => 0,
   

   Using boolean FALSE instead of 0.

3. +++ b/src/Feeds/Processor/Form/DefaultEntityProcessorForm.php
   @@ -118,6 +118,15 @@ class DefaultEntityProcessorForm extends ExternalPluginFormBase {
   +    if ($entity_type->entityClassImplements("\Drupal\Core\Entity\RevisionableInterface")) {
   

   Using a single quote instead of double quotes.

and I have added a test which toggle on and off the revision and check if the revision was created or not based on the toggle value.

Forgot to set the status to need review.

See MR for review comments.

I have not tested this functionality, what follows is pure code review.

There is one outstanding thread on the MR where I asked for dependency injection instead of side loading but @nikathone rightly brought up backwards compatibility as a potential issue - particularly as the class in question is a base class. I'm yet to discover how core handles this kind of change so I can't recommend a best practice.

Otherwise the code in this change looks good to me and the new test is solid so I'll say it passes community code review.

Leaving NR for someone to test the functionality out.

I had reason to try this out today. Doesn't look like it's functioning as expected.

I edited my feed to turn on "save as new revision" and the entities that I imported didn't get a new revision - note that they already existed as non-feeds items before I ran the import, for some reason they registered as "created" rather than "updated".

Possible that's a separate issue?

@darvanen, what are the settings for this content type for "allow revisions"? I believe that this configuration will impact how entities are imported.

@irinaz it's a custom entity that is definitely revisionable. In fact, it appears that the existing entity and its revisions are being overwritten by feeds. Needs more investigation I think.

Rebased the MR code and started to use the introduced dateTime property.

Needs a rebase before it can be effectively reviewed.

Yes, it looks like the rebase did not go well. When I go to https://git.drupalcode.org/project/feeds/-/merge_requests/37/diffs, I see a huge amount of changes, a lot not related to this issue. Best it would be to do the following:

1. Checkout latest 8.x-3.x
2. Create a new branch.
3. Cherry pick the commits belonging to this issue.
4. Force push this new branch to feeds-2926744/2926744-allow-revisionable-entities

It makes it also far more easier to review than when commits are "hidden" between commits from the 8.x-3.x branch.

I suspect the commits to pick are 81983bef, a9950c7b, 62c8b2c0, 2737d28f and cc70372d as these are all authored by @nikathone and don't have the "Issue #x" as prefix in the commit message, but I wanted to make sure that don't miss one.

My apologies @nikathone, I didn't see that you had already rebased. Thanks @MegaChriz for expanding.

Is it worth providing a choice of revision user? I can foresee feature requests around wanting the uploader to be the author of the revision in some cases.

I tried the patch from #8 and it did not work. Tried a CSV import with "create new revision" checked and instead of updating existing items, the module just reported that there were no new items and did not update anything (with revisions or otherwise). Following for now.

I tested the feature with revisions enabled and disabled and by mapping to certain revision fields (revision_log, revision_timestamp, revision_uid). I noticed that the mapping value for "revision_timestamp" was not respected, it got overwritten with the current timestamp. To fix that, in EntityProcessorBase::process() I moved the code that was setting a new revision to before the call to map() and then the revision timestamp could be set by the source.

@darvanen, #30

Is it worth providing a choice of revision user? I can foresee feature requests around wanting the uploader to be the author of the revision in some cases.

Nice suggestion, I think that this would best be handled in a follow-up. One way of implementing that would be to provide a FeedsSource plugin that returns the current logged in user. This could then be mapped to "revision_user". Though on imports during cron that would be user 0 I guess. Also, the source plugin has to be smart to pick the right user as a temporary user switch can happen during imports.

Right now, we have one single FeedsSource plugin that provides properties of the feed entity as mapping sources. They are listed underneath "Feed entity" in the screenshot above. (Note: the feature where sources getting categorized is provided by #3303166: Make source options more clear in the UI.)

@MegaChirz the latest changes looks good tom me and tested the MR to one of our website and everything works as expected. So I am plus one for RTBS but not sure if I should change it since I have also wrote some part of the code. Maybe @darvanen can do another review and RTBC if ok?

I merged the code! Thanks everyone who participated in this issue!

@nikathone, @darvanen
I've created a follow-up for setting revision user: #3305002: Revision author needs to be set
It would be cool if you can share your thoughts about that there, but I also consider that issue to be a lower priority. We could wait with diving into the issue further until someone really wants to have more control about the revision user being set. At least I've documented now what challenges there are for that feature.