Setting creation_date and modification_date of entries programatically is buggy

[michael-e]

When committing entries:

1. $entry->set('creation_date', $date) will only work if setDataFromPost is used in dry-run mode.
2. $entry->set('modification_date', $date) doesn't work at all.

@brendo already has a test scenario (that I created for #2398).

[nitriques]

Thanks Michael!

[brendo]

Looking at the code this is by design when using $simulate (dry-run).

When creating an Entry, it's desirable to use Entry->assignEntryId() which will create an initial row in the database and set the resulting auto_increment value as the Entry ID. Note that this function will set the creation_date and modification_date, but it doesn't set them onto the Entry object (bug).

Now, if this function is not run, it's ok, because the Entry->setDataFromPost function will automatically call it if the current Entry object does not have an id. This works as long as $simulate is false however in the test case provided for #2398 the $simulate flag is set to true, so the assignEntryId function is never called.

Now the date settings can be set programmatically ($entry->set('creation_date', date-here)), and this would work... but then there's another twist in the tale. The Entry->commit() function is used to actually persist the data to the database. This in turn calls Entry->findDefaultData(), which will override the modification_date value and set the creation_date if it's omitted (which it will be in a simulate scenario - unless it's added programmatically).

Now if the id is never set for the Entry, EntryManager::add() is called, passing the Entry object. add() will insert the entry data into tbl_entries, essentially just how assignEntryId() works, but it will not set any default values.

So to circle back:

• setting modification_date programmatically is ignored, as it's always overridden
• assignEntryId() creates the initial row, but doesn't set the creation date, modification date or author ID on the Entry object
• there's some duplication going on between Entry and EntryManager classes which could be tidied up

@michael-e Can you clarify "doesn't work"? I'd expect that the modification date set programmatically would be ignored, but if creation date was set, that should be persisted the database.

[michael-e]

$entry->set('modification_date', $date) doesn't work at all.

I meant that it is ignored, no matter if dry-run mode is used or not. It is always ignored.

$entry->set('creation_date', $date) will only work if setDataFromPost is used in dry-run mode.

Was supposed to say: Using dry-run mode I can set it programatically. Without dry-run mode it is ignored.

Can you confirm this?

[brendo]

I meant that it is ignored, no matter if dry-run mode is used or not. It is always ignored.

Yep, it will always be ignored because Entry->findDefaultData() will override it. Confirmed.

Was supposed to say: Using dry-run mode I can set it programatically. Without dry-run mode it is ignored.

I can't confirm this, setting $simulate to true or false still allows the test case to programmatically set the creation_date.

[michael-e]

In the test case we have:

$entry->set('creation_date', DateTimeObj::get('Y-m-d H:i:s', '2000-01-01 00:00:00'));
$entry->set('creation_date_gmt', DateTimeObj::getGMT('Y-m-d H:i:s', '2000-01-01 00:00:00'));

In my environment, both definitely get ignored (and the current date will be used) when I do

$entry->setDataFromPost($fields, $errors, false)

(in line 87 of the test case). Can you double-check?

[michael-e]

1. Inability to set the modification date programatically:

   Looking at these lines I wonder if this is indeed intended behaviour. @brendo: Should we change this? (It would be pretty simple.)

2. Setting the creation date only works if setDataFromPost is used in dry-run mode:

   For new entries, assignEntryId will be called — but only when $simulate == false. The latter will always set the creation date and the author ID. Hmmm, I don't really understand the intention behind that, if feels more like a bug to me.

[brendo]

1. Yes, we can change this one pretty easily to not set the modification_date if it's already been set (similar to values below it)

2. assignEntryId is only called when $simulate = false, and there is not an existing id set on the entry. That's correct behaviour to me?

[michael-e]

1. OK, should I send a pull request to the 2.6.x branch? Or will you care?
2. You mentioned that running an importer script, for example, can be problematic for certain fields if $simulate = true. But currently the ability to set a creation date for entries depends on $simulate. I am not sure if this is logical. I mean, how will a developer who builds an importer (or any other script creating entries) know this? (Still, I can live with that if you say that this is the right approach. We are probably talking about edge cases here.)

[brendo]

1. Yep that's fine!

2. The $simulate should only be used, (eg. set to true) if you want to simulate the data being added into Symphony. If you actually want the data to be persisted to the database, it should be set to false (which it is by default?)

[michael-e]

2. Yeah, but then you can't set the entry's creation date. :-(

[michael-e]

This is why I used true. Data was saved anyway, by commiting the entry (which might be a bit hard to understand).

[michael-e]

Re: 1. Pull request sent—tested, of course. It is now possible to set the modification date of an entry programmatically.

Re: 2. I am OK with leaving it as it is.

However, as often, I have one stupid question left. Assumed that, in an extension or a background process, I fetch a Symphony entry in order to manipulate one of the fields, the re-commit the entry: Should I call checkPostData and setDataFromPost in this case? Or is it enough to commit()? (As you see, I don't really understand the meaning of these two funtions...)

[nitriques]

@michael-e @brendo I would be interested in the answer myself ;)

[brendo]

This is why I used true. Data was saved anyway, by commiting the entry (which might be a bit hard to understand).

The simulation is only specific to the setDataFromPost function. If commit() is called afterwards, it doesn't have any concept of what was sent to setDataFromPost and so it'll continue with it's job of persistence.

2. Yeah, but then you can't set the entry's creation date. :-(

Well yes, at the moment, the creation date is when the entry was persisted to the database. We can solve this by removing these lines and adding the author check to the findDefaultData function. findDefaultData already handles the setting of the creation/modification dates, and supports not overriding them if they have been set. This would a be 3.x change though.

Should I call checkPostData and setDataFromPost in this case? Or is it enough to commit()? (As you see, I don't really understand the meaning of these two functions...)

checkPostData is the validation step. It takes the data and asks all the fields to validate that it meets the requirements dictated by the field settings such as if it's required, meets the regular expression pattern etc.

setDataFromPost works on the assumption that the data is now correct because it's been validated. It pairs up with the field's processRawFieldData function to transform the data that was submitted via POST (or anywhere), into a format that matches the field's database tables.

So if you look at it like that, checkPostData is validation, setDataFromPost is transformation and commit is persistence.

Hope that helps!

[nitriques]

So if you look at it like that, checkPostData is validation, setDataFromPost is transformation and commit is persistence.

+1 Thanks for the clarification !

[michael-e]

Yep, thanks a lot, @brendo!

[michael-e]

Well yes, at the moment, the creation date is when the entry was persisted to the database. We can solve this by removing these lines and adding the author check to the findDefaultData function. findDefaultData already handles the setting of the creation/modification dates, and supports not overriding them if they have been set. This would a be 3.x change though.

Should we leave this issue open then? Or should we rather say "there's no real need for that"? I won't insist, because I also see the logic behind the current concept.

[michael-e]

We have a problem.

My fix which has been pulled leads to a very strange situation. When an entry is saved in the backend, the modification_date_gmt will be updated, but the modification_date will not.

It might be that my "fix" is s bit stupid. But there is one more thing that really bothers me: How the hell can it be that the system behaves so strange?

[michael-e]

var_dumping an entry object immediately before the lines in question shows some interesting inconsistency:

object(Entry)#60 (3) {
  ["_fields":protected]=>
  array(5) {
    ["id"]=>
    string(6) "512436"
    ["author_id"]=>
    string(1) "1"
    ["section_id"]=>
    string(1) "3"
    ["creation_date"]=>
    string(25) "2015-05-01T18:48:14+02:00"
    ["modification_date"]=>
    string(25) "2015-05-01T18:48:14+02:00"
  }
  ["_data":protected]=>
  array(7) {
    [10]=>
    array(2) {
      ["value"]=>
      string(10) "Ted Tester"
      ["handle"]=>
      string(10) "ted-tester"
    }
    [11]=>
    array(1) {
      ["relation_id"]=>
      int(512411)
    }
  }
  ["creationDate"]=>
  string(25) "2015-05-01T18:48:14+02:00"
}

The entry doesn't have any modification_date_gmt property. Nor does it have a creation_date_gmt, nevertheless the function attempts to set this.

[michael-e]

At least the above explains the behaviour with my "fix" (which should be reverted). There is a modification_date, so that will remain unchanged. The modification_date_gmt, however, will be set to the current date.

But the process of saving entries in Symphony is over my head.

[nitriques]

@michael-e I think we need, again, @brendo to give us some insights...

[michael-e]

I am also OK with giving up on this, I can use a custom field to achieve what I need. I pass the decision to @brendo.

[brendo]

Yes, in hindsight that fix doesn't work at all because it can't determine between the initial modification_date is not set for entry creation, and an edit scenario. In an edit, there's nothing to let us know if the modificaiton_date is 'dirty' or not. Interesting

[michael-e]

At the moment — working hard to get a project up and running — I have a real talent to open Pandora's box(es). But as I said, we might just quickly close this box, at least for now. I have implemented a custom field that gives me greater flexibility anyway.

[brendo]

I've reverted the fix for the meantime, but yes, you have a knack of discovering some deep flaws in our architecture! From experience, this problem seems to be something that I've seen handled by the concept of 'dirty' objects. Angular and Laravel (I'm sure others as well) maintain a state of the object, and then are able to different the properties to see if anything has changed.

I believe if we could do this, we could do 'if modification date is dirty, don't set it, otherwise set it to be the current time', which would solve our problem and have the code work in both scenarios (programmatically or via the UI).

[nitriques]

@brendo I'd love to have a dirty state. But those things are hard to get right. I'd would love to review any code related to this :)

[brendo]

Agree. My initial thoughts were to overload the Entry->set/setData functionality. Anytime that is used, the property is added to a dirty heap. On further investigation I don't believe this will won't because we actually use those setters internally to build the entries from within the EntryManager. This leads to a more complex 'dirty' check, where secondary calls to the setters are compared to existing data to determine the state. This may be able to be achieved simply by hashing the data and if the hashes don't match, flag that property as dirty. Maybe? What do you think?

[nitriques]

I really like the hash idea. But, from experience, the best dirty system I've build required dev to manually set the dirty bits. One could want to change the entries in memory but not persist the change.

Also, I would try to see if a global dirty bit would be enough. Or maybe per field dirty bit...

I do not know if comparing array is faster than hashing. Is easier to code tho.

[michael-e]

Keep cool. Apart from a performing a programming exercise, what would we win? We'd make a "naturally grown" architecture even more knotted. I'd suggest to revisit this issue when the time comes to re-write this architecture anyway.

(I am not even sure that setting these dates programatically is indeed desirable. It was just, well, I tried it…)

[nitriques]

LOL. Having this kind of system would enable perf optimisation. But hey, it's not a requirement...

[nitriques]

This is fixed in 3.0.0