EZP-25377 fix republish modifying publication date

[iherak]

Issue: https://jira.ez.no/browse/EZP-25377

When publishing existing content (creating new version) with the PAPI, the publication date of the content gets the current datetime, same as modification date. While modification date behavior is correct, the publish date of the content should remain unchanged to reflect the original (first) publish date of the content.

Publishing from legacy administration does just that - changes only modification date.

To reproduce:

• load content
• create draft
• modify content with updatestruct
• publish new draft

Expected outcome: original content and republished content have the same publishedDate but different modificationDate.
Actual outcome: republished content has different publishedDate than original content.

I have added the regression test, but please do let me know if it's ok, and in the right place.

[crevillo]

i would probably move this above for better readability, i mean, the ternary operation

[crevillo]

beside my comment, looks good to me. let's see what travis says :)

[andrerom]

+1

There was someone reporting that the logic on modification data is a bit wrong, I think the outcome of the discussion was that perhaps modified date should be set to the modified date of the versionInfo. However that discussion might have been brought up because of this issue with publication date which was confusing reporter in regards to what should be expected. Thoughts?

[crevillo]

Maybe... do you have a link to the report?

[emodric]

@andrerom Now that you mention it, I think you're right. it is only logical that modification date be set to modification date of the version which is about to be published.

[crevillo]

But you need to set it to the content too, right?

[emodric]

We're talking about content, yes :) So instead of setting the modification date to time(), it should be set to modification date of the version, right?

[crevillo]

But isn't the modification date of the version time() too?

[emodric]

Is it? What happens if the draft is published a day or two after is was last modified?

[crevillo]

When you publish it, isn't the modification of the version also changed? Isn't internalPublishVersion called in that case?

[emodric]

That's the question. Is that the expected behaviour? Or in other words, does publishing a version constitute as modification of the version? It needs to be verified in legacy. I'm more inclined to believe that the behaviour over there is correct (in case it differs from behaviour here) :)

But still, in theory calling time() two times could return two different values, so internalPublishVersion should probably just use whatever already is in VersionInfo object.

[bdunogier]

does publishing a version constitute as modification of the version?

I vote for yes. The status of the version is modified.
It would make sense if the content's modification date was set to the value of the draft's modification time. Calling time() twice might not be a big deal, but there is no reason to, since we have the value we need.

[gggeek]

@emodric I am not 100% sure that legacy behaviour is either consistent or logical. It was never documented anywhere, and you can see that different devs had different ideas about those dates by the simple fact that the naming in code and in the gui suggest different things...

@bdunogier I do not agree necessarily that the act of publication should change the modification-date of a version.
In my POV:
object creation date = moment of publication of v1
object modification date = moment publication of vN
version creation date = moment the version gets stored the 1st time (q: is it before 1st saving?)
version modification date = version gets modified (ie. field values get changed)

So if we have a draft version which we edited yesterday, and today we publish it unchanged, it would make sense to me that the object modification date is different from the version modification date.
(not 100% sure that editors would understand this as obvious if shown all the dates in the dashboard, but a tooltip might alleviate doubts)

As long as we do not have version modification dates for non-drafts which are later than object modification dates, everything should still make sense.

[gggeek]

Last but not least: @bdunogier we currently have no date which tracks any other objects events apart from modification of attributes (version publication): hide/show, set-state, set-section are, between others, all events which do not alter the object modification-date.
If you want to add yet another timestamp which tracks the last change done to an object including all possible changes to its state, I am not against it.

[crevillo]

@emodric: question could also be "should we consider that content has been modified y that version is in draft?" I'd say no if i thinking in with the final user will see. I mean, if that version is in draft, what visitors of the site will see is the last published version of the content...

In other words, suppose we have a blog post dated on 2016-01-14. This date is showed as last modified date to the vistitors. Editor creates a draft tomorrow. That draft is either never published or never approved... Should we tell the visitor of the site that last modification of what's beein seing is 2016-01-15 or whatever? I would say that shouldn't change until that new version gets published...

[iherak]

Hi all, interesting discussion here..

I am not sure how the legacy works, nor how it was originally imagined, but for what it's worth, I am inclined to agree with @gggeek's description.

If the version (draft) has been created/modified the day before it was actually published, I, as developer, would expect the content's modified date to be the one when it was published. And then, if needed, I can dig into the version info to find when was the version created and last modified.

[emodric]

@emodric I am not 100% sure that legacy behaviour is either consistent or logical. It was never documented anywhere, and you can see that different devs had different ideas about those dates by the simple fact that the naming in code and in the gui suggest different things...

Maybe, but we should at least make it consistent, both in eZ kernel and legacy.

My view of all four timestamps:

object creation date = moment of publication of v1
object modification date = moment of last modification of published version fields (i.e. this does not get changed when draft version fields are modified, it is only set to modification date of the draft when it's published)
version creation date = moment the version gets created
version modification date = moment of last modification of version fields

[crevillo]

i'm doing some test with legacy backend.
so, when you publish first version of the object,

Created = Modified = 14/01/2016 09:23 

(note spanish date format here)

Editing that content and saving draft. then

Content Creation Date = Object Modification date = 14/01/2016 09:23 (so, no changed)
Version 2 Creation Date = 14/01/2016 09:26 
Version 2 Modification Date = 14/01/2016 09:27

Publishing version 2

Content creation date = 14/01/2016 09:23 
Content modification date = 14/01/2016 09:30
Version 1 creation date =  14/01/2016 09:23 
Version 1 modification date = 14/01/2016 09:23
Version 2 creation date =  14/01/2016 09:26 
Version 2 modification date = 14/01/2016 09:30 (same as new content modification date)

So, it looks when you publish the version, the version modification date is also changed and that value is also used to set the new object modification date...

This is how it works now, but yep, interesting discussion :)

[emodric]

@crevillo So it's a combination of @gggeek's and mine POV :)

[crevillo]

@emodric and mine too :)

[crevillo]

there's another edge case with version modification date... don't remember how it works when you have ezautosave in place...

[emodric]

there's another edge case with version modification date... don't remember how it works when you have ezautosave in place...

Hm... Never used it myself, it was always one of the first things I disabled when installing eZ :)

But should it be considered at all, since it doesn't exist in the new stack?

[crevillo]

it doesn't, but maybe it will :)

[crevillo]

side note... and if i'm not wrong, content versioning will change a bit in new stack, won't it? i mean, i remember something about you will can create different views of the same content depending on browser settings like language, ip of the visitor... etc...

site note 2: actually there's also something to be considered with translation. actually when you add a french translation to a content with main language being english, content modification date gets changed too... but user visiting english version of the site won't see any change... right?

[gggeek]

@emodric I am not sure I agree with the fact that object modification date should be aligned with the date of last modification of version fields.
To me the object creation/modification dates always have meant 'the content dates as seen by the end user on the site'. So if you edit the version on wed but publish on thu, the date should say thursday.

Just consider that 99% of ez websites have some approval / delayed publication workflows. In both cases, what the end user has to see is the date of content/new version appearing on the site, not the date when fields were modified.
If we do not save in the object modification date this value, then we are forcing the developers to always add a custom 'object publication date' field, which is just more work

[crevillo]

exactly my point @gggeek :)

[emodric]

@gggeek Hm... Okay. Maybe you're right. But I still think that modification date of an object should be the same as modification date of the last published version, that is, publishing the version should update its modification date.

[iherak]

@emodric would you not then lose information about when were the actual changes on the fields (version modification date)? I am aware from what @crevillo wrote that what you are now describing is how legacy works, but personally I still think it would not be a bad idea to have that information?

[crevillo]

@andrerom if i'm not wrong, what happens before this patch is creation date is also updated when you modify the object (publish new version).

[andrerom]

@crevillo what do you mean by creation date? As far as I remember metadataUpdateStruct acts on the Content where there is no such thing, and does not act on the Version.

[crevillo]

@andrerom With @iherak's patch applied, after publishing the content and going to details tab i can see

Creator Administrator User (2016-01-13T17:06:02.000Z)
Last contributor Administrator User (2016-01-14T08:10:18.000Z)

i guess those are content (contentInfo) creation and modification dates, aren't they?

Without the patch, i republish this content and i have...

Creator Administrator User (2016-01-14T16:32:51.000Z)
Last contributor Administrator User (2016-01-14T16:32:51.000Z)

So, looks like last modification and creation date are the same.

[iherak]

On content, it's called "publishedDate", but yes, it should show the time of the first publishing. That's how it behaves in legacy AFAIK, does it not?

[crevillo]

and btw, if you look the database without @iherak patch, now i can see published date has been modified.

mysql> select published, modified, FROM_UNIXTIME(published), FROM_UNIXTIME(modified) from ezcontentobject where id = 1;
+------------+------------+--------------------------+-------------------------+
| published  | modified   | FROM_UNIXTIME(published) | FROM_UNIXTIME(modified) |
+------------+------------+--------------------------+-------------------------+
| 1452789171 | 1452789171 | 2016-01-14 17:32:51      | 2016-01-14 17:32:51     |
+------------+------------+--------------------------+-------------------------+

[andrerom]

@crevillo there is no concept of created date for content object in kernel, what labels they use in ui and if that is misleading is a bug there.

[crevillo]

@andrerom no published date either?
then i start to think i have no idea on what's going here. :)

Edit: Leaving this platformui question for @dpobel or maybe @rolandbenedetti, i ask. is ok that published column in ezcontentobject table gets updated for every new version of the content? if it's ok then i said nothing. but don't know how we could make different lists if we sort by published or modified...

[iherak]

To add to what @crevillo asked, if that is so, does that mean that there is no proper way of having the date of the first publish?

[emodric]

Object published date has always been the timestamp of first publish. Once the object was created, it was never modifed again.

Object modified date has always been the timestamp of last publish.

[crevillo]

according to what @emodric says (and I second) we should

• decide is this needs to work in the same way in kernel or not
• test if the error come from platform ui or is something that always happens, with a command or something.

[andrerom]

I would rather remove the changes in publishVersion and do something like this on this line to make sure all use of this have the same behaviour everywhere, and be closer to how legacy behaves:

if ($publicationDate === null && $versionInfo->getContentInfo()->publishedDate->getTimestamp() === 0) {
    $publicationDate = time();
}

$metadataUpdateStruct = new SPIMetadataUpdateStruct();
$metadataUpdateStruct->publicationDate = $publicationDate;

php doc something like:

@param int|null $publicationDate If null existing date is kept if there is one, otherwise current time is used.

[emodric]

@andrerom It should probably be:

if ($publicationDate === null) {
    $publicationDate = $versionInfo->getContentInfo()->publishedDate->getTimestamp() ?: time();
}

Right?

EDIT: Disregard. My snippet is wrong.

[andrerom]

The value is allowed to be null, ref the meta update struct php doc.

[iherak]

@andrerom would something like this work:

if ($publicationDate === null && $versionInfo->getContentInfo()->publishedDate === null) {
    $publicationDate = time();
}

From what I gather, the published date is never set to timestamp 0, but to null (when I tried your snippet, got exception, so I tried to dig around a bit).

Alternatively, we could be a bit more explicit (while preserving the desired behaviour), and do something like this:

if ($publicationDate === null && $versionInfo->versionNo === 1) {
    $publicationDate = time();
}

This way it would be obvious from the code that we are setting the publication date to current time only if it is the first version, and publicationDate has not been set manually?

[andrerom]

Both are fine with me, and second is indeed more readable so +1 from me on that.

[andrerom]

I'm lost in the conversation so I'll rather focus on this PR now and if there is any further questions beyond this PR we can find another time and place to deal with that.

---

@iherak Had a second look here and in legacy, and added inline comment to suggest to adjust the approach slightly. However logic change is more or less what you suggests, just closer to how legacy does it and placing it in internalPublishVersion.

Should probably also add a BC not btw, even if this makes platform kernel behave closer to legacy.

[iherak]

@andrerom I have added the note about the BC, please let me know if it's ok, or should I reword it or reformat it in some way.

[andrerom]

bc note is ok

[bdunogier]

Is the PR's description up-to-date with the latest changes ?

[iherak]

Yes, it is.

[andrerom]

+1 on 3378307
(on merge bc note will be moved to 6.1 as that is now the next tag for master, I would also like to back port this to 5.4 given the previous behavior is not in sync with legacy, but that is a discussion me and @bdunogier can have separately)

[bdunogier]

"to the publishing time of", or "to the latest version's publishedDate" ?

[iherak]

The former. I can reword the note so it is more obvious.

[bdunogier]

+1

[andrerom]

It's in @iherak, celebrate 🎆 ;)

[iherak]

Great, thanks!
Happy to see it will be backported to 5.4 as well :)