Add 'published' timestamp to nodes

How can I help?

Typo in one of the comments: // Set the pulished timestamp to...

Given its scope, patch looks fine to me.

Question is whether the node table is the right spot for the published flag (status) and the publication date or whether it should be the node_revision table. Or whether this should be generalised further to cover any entity...

I think its a good idea though we have to also consider if there are any ripple effects through the rest of the system (does it show up in views, etc). I don't think this is the kind of change that can get a backport to D7. With that said, we do need tests (both regular to test that the published timestamp works and upgrade path to make sure the timestamps are accurate).

The node save test will need to be updated for the published date. We also need views tests.

For upgrade path tests, the simplest thing to do would be look at the basic upgrade path tests (look at something like http://api.drupal.org/api/drupal/core!modules!system!lib!Drupal!system!T... (look at the source near the bottom of the page) and you'll have something like that and you would be changing up the confirmation tests after the upgrade.

I can help out with the upgrade path tests and will try to get them sometime today (not changing the assigned since you have it but just an FYI).

Sorry I couldn't get to it earlier. The plan is to tackle this sometime over the weekend (both the review and attaching an upgrade test).

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeSaveTest.phpundefined
@@ -72,7 +72,7 @@ function testImport() {
   /**
-   * Verifies accuracy of the "created" and "changed" timestamp functionality.
+   * Verifies accuracy of the "created", "changed" and "published" timestamp functionality.

This needs to be a one line summary less than 81 characters. Perhaps re-word with a second paragraph of what fields/properties are checked?

I'm attaching a version of the patch that is just with the upgrade test and one that is combined with the current patch. The upgrade test is fairly rudimentary but we have a base to start from. Test seems to pass so far. But needs further review.

#14: published-timestamp-1838918-14.patch queued for re-testing.

I'm a bit confused by how my patch failed if its the same but with an additional test. Retesting.

#15: 1838918.patch queued for re-testing.

There we go - now passes and actually 'needs review' :P

Do we need to declare the new property in the typedData system? Are other date fields handled with typed data?

The update part of patch seems flawed to me. Using the changed date seems incorrect as nodes could have been published and then updated multiple times after that initial save, I think there needs to be consideration as to whether or not a node type has revisions, in which case the date of the first revision at status 1 should be used as the published date.

Also node_update_8014 should run as a batch.

@jstoller: I am just starting with the D8 testing and this the first patch I ever tested.
I applied it to my local git install and have some results, could you check them out and leave your comment?

• When editing an unpublished with an published timestamp:
• Leaving it empty does not use form submission as the description suggests but uses the old value
• entering 29-12-28 11:33:38 +0100 returns 2029-12-28 11:33:38 +0100

The last issue might not be right for this patch because you use the code below and not actually check it yourself.

   // Validate the "published on" field.
   $pub_date = new DrupalDateTime($node->pub_date);
    if ($pub_date->hasErrors()) {
    form_set_error('pub_date', t('You have to specify a valid publication date.'));
    }

If you need help with coding I am happy to help, I have experience in D5/6/7 but have not worked on 8 yet.

The upgrade path still needs some love and I forgot about this issue :( Have a look at taxonomy_update_7005 for an example of a batch update.

Also why do this

+    if (!empty($node->published)) {
+      $node->pub_date = format_date($node->published, 'custom', 'Y-m-d H:i:s O');
+    }

Instead why not format date at render time as internally we are likely only interested in the unix timestamp.

The use of node publish and pub_date in the FAPI code seems strange and if you just went done the root of formating publish it would seem cleaner to me, also maybe consider allowing the format to be over riden via $conf.

Can this

if (!empty($node->pub_date)) {
+    $node_published = new DrupalDateTime($node->pub_date);
+    $node->published = $node_published->getTimestamp();
+  }

Be simplified to

if (!empty($node->pub_date)) {
$node->published = new DrupalDateTime($node->pub_date)->getTimestamp();
}

Also there is the queston of views support. This adds a new 'field' but no where in core is it actually exposed.

Sorry to just jump in here, hope that's ok. Now Views is in core, this should probably include the integration for that too, as it should be relatively trivial. I also just removed a couple of whitespace characters from the last patch.

err, yeah, I would have rolled in my changes for the actual integration too ;) See the interdiff in #34. So my feedback on that is basically contained in what I have done, that you didn't roll into your patch :)

It's in there, but clearly my patch wasn't used. You have just done it yourself I guess?! Things like the inline comments? Which we don't do anymore. Generally it's a good idea to just use the most recent patch, rather than talk about changing an older one to have changes from a newer one :)

This is really good so far but i really wonder why this is tagged as needs backport. Isn't that a new feature?

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -44,6 +44,9 @@ protected function prepareEntity(EntityInterface $node) {
+      $node->pub_date = format_date($node->published, 'custom', 'Y-m-d H:i:s O');

In general i'm wondering whether we should really shorten here and not use something like $node->published_date

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeSaveTest.phpundefined
@@ -102,29 +106,75 @@ function testTimestamps() {
+      'status' => 0,
...
+    $node->status = 1;
...
+    $node->status = 0;
...
+    $node->status = 1;

What about using the node status constants.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeSaveTest.phpundefined
@@ -102,29 +106,75 @@ function testTimestamps() {
+    $node = $this->drupalGetNodeByTitle($edit['title']);
...
+    $node = $this->drupalGetNodeByTitle($edit['title'], TRUE);
...
+    $node = $this->drupalGetNodeByTitle($edit['title'], TRUE);
...
+    $node = $this->drupalGetNodeByTitle($edit['title'], TRUE);
...
+    $node = $this->drupalGetNodeByTitle($edit['title'], TRUE);

Why not just get the node from the entity_create and not load it all the time. This could then be a lot easier.

+++ b/core/modules/node/node.moduleundefined
@@ -992,6 +992,17 @@ function node_submit(Node $node) {
+  elseif (!empty($node->status)) {
+    $node->published = REQUEST_TIME;
+  }
+  else {
+    $node->published = 0;
+  }

Don't we have the same logic in the storage controller already?

+++ b/core/modules/node/node.views.incundefined
@@ -117,6 +117,22 @@ function node_views_data() {
+  // published field
+  $data['node']['published'] = array(
+    'title' => t('Published date'), // The item it appears as on the UI,
+    'help' => t('The date the content was first published.'), // The help that appears on the UI,

As damian wrote, there is no need for this silly comments anymore and we remove it in other patches, but yeah you couldn't have known that.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/NodePublishUpgradePathTest.phpundefined
@@ -0,0 +1,51 @@
+class NodePublishUpgradePathTest extends UpgradePathTestBase {

Oh great, an upgrade path test!

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/NodePublishUpgradePathTest.phpundefined
@@ -0,0 +1,51 @@
+   * Tests a successful point release update.

Better describe what is going on here, it's updating the nodePublished column.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/NodePublishUpgradePathTest.phpundefined
@@ -0,0 +1,51 @@
+      $timestamp = db_query('SELECT timestamp FROM {node_revision} WHERE nid=:nid AND status=1 GROUP BY nid', array(':nid' => $node->nid))->fetchField();

You probably should also use spaces between nid=:nid etc.

This is certainly more performant, but in order for this to be a true test I think we need to first save the node and then read it back out again, using complete save and load operations.

Is there anything else in core which does something similar? If you really need the loading why not simply use $node = node_load($node->nid);

In general I think you should wait until #1833334: EntityNG: integrate a dynamic property data table handling is in, because from my perspective a generic entity storage is way more important
than this issue (sorry) and so we shouldn't work towards breaking that patch.

This was the totally wrong issue: #1498674: Refactor node properties to multilingual is the one I was talking about.
As that issue hasn't been updated that recently you should probably not wait on that one, but that is just a guess.

It's generally looking pretty good, a couple of things:

+++ b/core/modules/history/lib/Drupal/history/Plugin/views/field/HistoryUserTimestamp.phpundefined
@@ -37,6 +37,7 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+      $this->additional_fields['published'] = array('table' => 'node', 'field' => 'published');

This has crept in, I don't think we need this on this handler? I know it completes the set but it's not being used.

+++ b/core/modules/node/node.views.incundefined
@@ -382,6 +398,60 @@ function node_views_data() {
+  $data['node']['published_fulldate'] = array(
...
+  $data['node']['published_year_month'] = array(
...
+  $data['node']['published_year'] = array(
...
+  $data['node']['published_month'] = array(
...
+  $data['node']['published_day'] = array(

Not too sure what all these are doing in here? The Date handler for the published field data will handle outputting in whichever format you need. Plus, none of these plugins, such as 'node_published_fulldate' even exist, So this would be throwing some errors :)

Sorry, arguments not filters :) Just the first comment, not the second! Sorry!

So after looking at the upgrade path there is an issue. The process is based off using a timestamp off of the node revision table. This works prefectly if revisions per node are enabled otherwise you end up with a case as follows.

I create a node that is status one. 2 weeks later I change some text the timestamp gets updated. The first published date will now be incorrect. I am not sure what the solution to this but as it stands the approach being used could lead to massive problems. Maybe we need to follow these rules

* If we have revisions use the first status 1 entry
* If we have no revisions use the created date, while this also may be wrong it means that the it adheres to the current rules where created is given precedence over changed.

Also I would like to weigh in that this can't really be back ported to drupal 7 in my opinion as it has significant implications in terms of upgrades for existing sites and the same solution can be achieved by adding a field in d7 and populating that in a programatic way so while a core solution might be nice it this is really a feature request.

I will let you guys fight out the upgrade path tests. when those are sorted, the rest is rtbc for me.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeSaveTest.php
@@ -102,29 +106,75 @@ function testTimestamps() {
+    $node = $this->drupalGetNodeByTitle($edit['title']);
...
+    $node = $this->drupalGetNodeByTitle($edit['title'], TRUE);
...
+    $node = $this->drupalGetNodeByTitle($edit['title'], TRUE);
...
+    $node = $this->drupalGetNodeByTitle($edit['title'], TRUE);
...
+    $node = $this->drupalGetNodeByTitle($edit['title'], TRUE);

This should be $node = node_load($node->id()).

+++ b/core/modules/node/node.install
@@ -707,6 +713,97 @@ function node_update_8013() {
+      ->condition('r.status', 1,'=')

1. There should be a space after the last comma.
2. I might be mistaken, but I think '=' is the default condition operator and, hence, can be omitted.

Both of these things are very minor, and this could definitely go in, without that, but I have one more substantial question:
I am far from being a DB expert so this might actually have a valid answer, but is there a reason that moving all nodes into the temporary table can be done in a single pass, but then moving that back into node requires a batch?

Another thing has crossed my mind after looking at the actually form changes in a browser which seems to create a bit of wtf from a user point of view. We have 2 field exposed
* Created
* First published

The 2 can be set independently and you can infact set a nodes creation date to be after the publish date which seems strange. Now maybe this is part of a follow but the UI we end up with is less than ideal. Maybe this patch should be split into 2 part one
* Add the new field and make sure that it gets populated when the node is first published but with no exposure on the node form
* Step 2 amend the UI to expose the field and likely hide the creation date of the node as that is now in effect behaving in a similar way as being changed.

As I say it might be possible to commit this patch as is and do the created/published changes in a follow up so not blocking as such just throwing the idea out there as I am sure this patch will get some more reviews once it hits the rtbc queue.

Otherwise I think the patch looking pretty good, and I agree it would be nice to get someone with some database chops to look at the update function detail.

I will mark rtbc but I have real concerns about the ux of what is being committed, I am sure though who ever commits this will weigh in if they have issues with UX.

I do see what you are saying re UX and I am not saying publication date can't be before created date, what I am saying is that it makes no real sense to expose both in the UI as it opens up users to ask questions, and remember not all sites are configured by true site builders or developers so there needs to be a sane and usable default.

Yup I am in agreement with the end state of just published, and hiding the created date, so I think we are seeing exactly the same end state :) Did you cross post or did you mean to set it back to needs review?

Agree with the RTBC code-wise. Thanks for your explanations @jstoller, they make tons of sense :-)

Reading the last comment, I think this could use a lookover by Bojhan, yoroy or someone else from the usability team, to make sure we're not introducing a regression on one of *the* most visited forms on most Drupal sites. Hence, moving to "needs review" and tagging accordingly.

Also, to accelerate the usability review process, it would be awesome if someone could attach before/after screenshots of everything that this patch changes in the UI.

Again, if we get the green light from the UX team, this can go straight to RTBC.

(Also removing the backport to D7 tag, I highly doubt we will want to do that.)

Edit: Remove completely weird English. Sorry, not a native speaker.

Should that say "First published on"?

Knowing some users, I think there might be some confusion about whether this is a scheduled publishing field, ie if they uncheck the "published" checkbox and enter a future date/time into the field, they'll think it will be automatically published at that date/time. But that's something the usability review can consider, and I'm unlikely to let my users anywhere near this option anyway ;-)

For what it's worth, I agree with #72. If I encountered this in the wild I would wonder if it supported auto-publishing on future dates, so I suspect end-users might easily assume it.

I foresee a bigger problem in the long-run if workflow statuses are abstracted out. If a node can be "In review", "Rejected", etc. then editable date fields for each becomes unwieldy. Even now, the argument for a published timestamp could be applied to the "Promoted to front page" and "Sticky at top of lists" states much the same.

I think having this feature is a good idea. The latest UI is not yet convincing though:

- The UI elements are in the right place. But boy do they fill up the room.
- Is the 80% use case that you want to use this? Or should this be something that's available when needed but not so prominently there all the time?
- The amount of description text is a clear indication that a single text field does not suffice for helping people express a full data and time. One would expect a date picker here.
- The wording of the label reads as if this is required info, almost as a direct order. What I miss is a clue for why I might want this: "Change original publication date" or "Set first published date" or something.

Overall I think this introduces too much UI too prominently. "Nice to have", maybe "should have" but the current UI presents this in a "Must have" fashion.

What about showing the actual "first published on: date, time" info, with the date and time as a link that trigger the text field or date picker? Similar to the machine name pattern: http://drupal.org/node/1227546

My review was very much with the new content creation form in mind :) I agree date picker is beyond current scope here.

I don't understand your reasoning around abstracting around machine name pattern. It's already a pattern, what further abstraction is needed here?

Does the field show a value when editing an existing content item? It's the emptyness of the field that makes this look, not necessarily required, but leaving it blank suggests you are forgetting or ignoring something. It leaves on open loop in the user experience, which doesn't build trust.

So, current UI is not acceptable in my opinion. A usability review was requested, I gave one. It's not encouraging to then get 'lets make that a follow up issue' as the response. I'm fine with getting the feature in first but then it may be better to remove the UI all together and design a better one in a followup.

Personally I'd expect the "first published on" field to be non-editable for most users anyway (a permission perhaps?), and for it to display "never published" if the node has never been published.

Yup, happy with the solution proposed in #80.

The UX in #80 is impressive! Let’s get this in.

Updated issue summary.

Don't let this delay approval: a question I know one of my clients will ask: if the first-published date is overridden in a later revision, is that a property of the node or of the node revision? In other words can we still recover the "real" first-published date?

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -258,6 +261,40 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
+        'style' => array('display:none;'),

Why would we do inline styling here, that's not really nice if people want to override, this should go away completely. - edit - or use the 'element-invisible' class.

Note that #1751606: Move published status checkbox next to "Save" will probably go in this week which removes the 'published' checkbox completely - although that probably won't interfere *that* much I guess.

Oh, right fine enough as well, I knew it was something with 'element' :)

+++ b/core/modules/node/node.views.incundefined
@@ -383,6 +399,60 @@ function node_views_data() {
+      'id' => 'node_published_fulldate',
...
+      'id' => 'node_published_year_month',
...
+      'id' => 'node_published_year',
...
+      'id' => 'node_published_month',
...
+      'id' => 'node_published_day',
...
+      'id' => 'node_published_week',

Instead of using plugin IDs which does not exists yet, you should probably use the ones coming from #1893906: Move views argument date handlers from node module

It seem so to, as the other patch is already RTBC, that it will committed before.

Not sure whether I described that correctly, but the code from above simply does not work at all ;)

This fixes looks good!

No issues from me about rtbc'ing this now it has usability review assuming yoroy is happy that is!

I still find it unsettling that when checking the 'Published' checkbox this additional options shows up. You check the box to mark the content as published. Not to change the first publication date. I understand that that's when the option becomes available, but just checking the 'Published' checkbox does not mean there's an intent to change the 'first published' date.

Also, I worry about a mental clashing of concepts with scheduler-style modules, especially when this 'first publised' option gets shown when the node hasn't been saved yet. Can we offer this option only after the content has been published?

**Edit: updated wireframe in next comment**

Forgot last step in wireframe.

Not showing the published date until after the publish event has actually occurred seems like a win. But any kind of editable date field here definitely implies a scheduler. What about either:

A - not allowing a first-published date to be set which is in the future, or
B - just display the original first-published date, and leave making it editable to contrib?

Yes, we've had an editable date field on the node form forever. It's also been quite well hidden all the time.

- I still think this should be a link, not a checkbox. Ideally, a checkbox is used for a on/off type setting. A link would be less UI chrome and better convey the nature of the interaction: you click the link because you want to customize this, so the resulting extra UI elements are less of a surprise.
- How do you think this feature should look when #1751606: Move published status checkbox next to "Save" is in? Since this is indeed the state we're working towards I'm still very uncomfortable with adding this "temporary" UI.

Current proposals put thing very prominently in sight around the arguably most important functionality of a CMS: getting stuff published to the internets. Right now, it clutters that part of the UI too much. We have to be very picky here.

So, assuming #103, could the "Authored on" field (created date field) simply be replaced by "Published on" (published date field) and let contrib modules deal with exposing the created date? Having two editable date fields creates a UX quagmire for the most common publishing use case (which only display one date on the front end) and will be the source of lots of confusion.... "Wait, which field should I edit to change the display date on the front end?"

“Authored on” and “First published on” does not mean anything and are confusing.

Do you mean the “created” date? Or the new “published” date?

Please remove any confusion and use simple and understandable lingo. Also think of translation issues when something is not clear, translation will add another degree of uncertainty.

We should stick to Created [date], Modified [date] and Published [date].

I still don't understand that “First published on” label. Is this the creation date, or the new publication date?

When creating a node, it may not be published at the same time of its creation.

Last saved means last modification, so modification date. That one is fine.

So is First published on the new publication date? If so, why do we let the end user changed it's value? Because changing the value would not make it First published on.

If First published on is the creation date, well, change the label back to Creation.

Please remove any confusion and use simple and understandable language. Also think of translation issues when something is not clear, translation will add another degree of uncertainty.

As per my previous comment.

The problem is that "publish" has years of accumulated meaning in Drupal. I'm not sure there is a useful synonym however...

Authoring something = creating something, hence Authoring on == Created on. This is specially true in the content creation industry, where the author creates content.

Changing “authoring on” to “published on” is bending their definitions of in the English language.

The problem is within Drupal since its beginning, where content publication was done at the same time as it creation. One flag is missing 'publication date', so users often changed the 'creation date' manually through the UI. We need a 'publication date' in the database.

On a content management system, or publication-centric system used in newspaper, for example, creating content did not mean publishing the content. An article might be written one day and published the day later.

So until the database as one more row in the node table (we currently have created and changed date), we should be warry about using any published on date.

Let's add a new “published” date row in the node table.

But for now, the label should either be:

- Created on
- Authored on

....?

That's what this patch does...?

@webchick: This patch merges the creation date with the published flag and add a new published date in the database.

+++ b/core/modules/node/node.install
@@ -114,6 +120,7 @@ function node_schema() {
       'node_changed'        => array('changed'),
       'node_created'        => array('created'),
+      'node_published'      => array('published'),
       'node_frontpage'      => array('promote', 'status', 'sticky', 'created'),

This is a huge UX improvement!

But I fail to see which field is displayed here under the First published on field (created or published).

The proper label discussion is very minor compared to the large usability and richness of that this patch brings to Drupal.

Updated issue summary.

Now that created is gone it remove the UI elements that I saw could cause confusion (note am I not a ux person in anyway shape or form). It would be a shame if this patch missed the code freeze because of bike sheding.

I really hate stirring the dust on this, but I'm trying to think of publishing workflow and I'm also now coming up short... please hear me out (and shoot me down if necessary ;) These observations could be moved to a follow up ticket, because I dont want this to block having this initial improvements getting in in the first place.

So, if not "First published on" then how would you recommend we label this field? What would better imply that this is the date of the node's initial publication?

I find the "First published on" label confusing for two reasons:

1) "First published on" when starting from a new content, and the field is empty, insinuates (incorrectly?) that if you populate the field with a date in the future upon first save of the node, it is scheduled to be published on a date in the future. Does this patch actually do this? If not, we might consider providing an additional sentence in the description text, when the item is new and the field has no value, that "Note: specifying a date in the future will not schedule this for publishing". This will leave room for a contrib module (or a follow-up core issue) to provide this extra workflow logic and change this description text text to say that specifying a date in the future will schedule the node for publishing.

UPDATE: I just thought about this again, and its less about the label, as the field in general.

2) "First published on" loses meaning with reality as soon as you change the default auto-populated value in a subsequent edit after having published the node for the first time. I understand the reasoning behind why the word "First" was added above, but I think that this is a red herring that makes things more complicated to understand and will generate more questions. To me, this is a simple problem with a simple answer: the fact that we are exposing this field as editable means there can only be one value, and by default, it is not updated in subsequent unpublish --> publish operations.

One possible solution to this would be to provide an additional sentence in the description text, when the item is not new and is unpublished but already has a published date, stating that the field is not automatically updated when clicking the save/publish button, and that it is exposed as editable for just this reason. This would leave room for a contrib module to eg, remove this extra text, make the field "click to edit" or hide it altogether, and provide the additional workflow logic someone may want to have automatically updated publish dates on subsequent content workflow edits.

Since these are just observations, I don't want to block progress, I'm not going to change this back to needs work unless someone else agrees with the sentiments here ;)

A final suggestion, in an effort to keep the field description text shorter, we could wrap the example date in an abbr (or some other more semantic field) and put the text "The date format is YYYY-DD-MM and -0800 is the time zone offset from UTC" as the *title* attribute, that appears when you hover the example date.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -41,7 +41,7 @@ protected function prepareEntity(EntityInterface $node) {
-      $node->date = format_date($node->created, 'custom', 'Y-m-d H:i:s O');
+      $node->published_date = format_date($node->published, 'custom', 'Y-m-d H:i:s O');

+++ b/core/modules/node/node.moduleundefined
@@ -1012,6 +1012,14 @@ function node_submit(Node $node) {
+  if (!empty($node->published_date)) {
+    $node_published = new DrupalDateTime($node->published_date);
+    $node->published = $node_published->getTimestamp();
+  }
+  else {
+    $node->published = 0;
+  }

Why do we need to save the published timestamp in a formatted date that is apparently only used on the edit form? Could the translation from timestamp format into the form field format be done without the need for an additional public member on the $node object? What does having this duplicate information buy us?

For what it's worth, my personal opposition to this is because the majority of the workflow systems I build are entirely private, where private-to-public content publishing concepts simply don't apply. This proposal would background metadata that applies to most content and replace it with a permanent UI element that will be irrelevant, and forever blank, for many non-default content types. I'd rather this wasn't railroaded through on a whim.

It is incorrect to simply reuse the "Leave blank to use the time of form submission" text from the "Authored on" description. This happens to relate directly to the first functional problem with the patch that I can see: when I select "Save as unpublished" for a new node, the "First published on" date comes back filled with "1969-12-31 16:00:00 -0800". I haven't looked into it in depth, but I figure this is because the published_date is always being passed through format_date() in prepareEntity().

so, NW, based on the issue brought up in #139, which seems sound, no?

+++ b/core/modules/node/node.installundefined
@@ -707,6 +713,97 @@ function node_update_8013() {
+      // See if the node has multiple revisions. If it does, use the revision
+      // timestamp. If not then use the node created date.
+      $revisions = db_query('SELECT COUNT(*) FROM {node_revision} WHERE nid = :nid', array(':nid' => $nid))->fetchField();
+      $published = ($revisions > 1) ? $record->timestamp : $record->created;

Can you clarify why you'd pick the revision's timestamp date as the published date when first processing and adding this new field to the database, but when a new revision gets created through Drupal's UI, you specifically do not update the published date.

Also, we've already got the node's first published revision timestamp stored in the temporary table. First, if there was only one revision, and the node's created date was not changed by hand, then the revision->timestamp and node->created should be the same value no? Could that logic be used instead of an extra query to get the number of revisions? Secondly, if there's more than one revision, and the first published revision's timestamp doesn't match that of the original $node->created date, then how can you be sure that you're picking the right date to put in the new published field? Does Drupal display the revision's timestamp (if it exists) in place of the created date on the front end for the $submitted data? From the non-developer site content administrator's perspective, this will have the effect that the custom created date that may have been specified on the node at some point is inadvertently changed/updated to some other seemingly random value.

Needs reroll ... conflicts with f6b4d4a22.

Affected files:
core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.php
core/modules/node/lib/Drupal/node/NodeFormController.php
core/modules/node/node.install
core/modules/node/node.module
core/modules/taxonomy/lib/Drupal/taxonomy/Tests/LegacyTest.php

The NodeFormController and node.module changes could use a second set of eyes ... I'm admittedly guessing, since I haven't looked at the new DateTime piece, and wasn't sure if the fixed formatting in the published portion was done for a reason or not.

@jstoller Not sure how to do that, if the original patch doesn't apply. That said, the conflicts were largely minor ... with the biggest change being that the date property is now a DateTime object instead of a string. NodeFormController.php and node.module are the only files that weren't absolutely trivial.

Edit: Long story short ... I only spent about 5 minutes on it; no real lost effort if you simply applied the reroll yourself. Most of the fails would be due to tests keying on the parameter which was replaced with $node->published; though I wasn't sure if your patch intended to replace the original parameter, or add a new one.

un-tagging

Updated issue summary.

Uploading again for testbot, no change from #163

Sorry, being stupid, forgot to set to needs review. This will certainly fail.

https://github.com/ueberbit/publication_date has a contrib solution to this that uses a translatable non-revisionable basefield to deliver the same result as this patch, but in a way that can be applied to any entity.

Hi, Has this issue died? Seems kinda odd to me not to have this in core given that it's a basic requirement for editorial sites which work on content and revise it in advance of publication. The Publication Date module hasn't moved out of Beta so this requirement still seems essential to me.

We have content moderation in Drupal core now so the problem space gets even more complex. Until its solved, I suggest creating a regular Publish Date field on your entities and manually setting a value, or add some code to programmatically set it. This would all be done by a Contrib module as well.

There is a helper module https://www.drupal.org/project/publication_date

+ 1 for this as trying to do basic stuff like this kinda sucks.
$database->query("SELECT DISTINCT YEAR(published_at) from {node_field_data}"); .. :( is int field

So have to do ..
SELECT DISTINCT YEAR(FROM_UNIXTIME(published_at)) FROM node_field_data

@taggartj re #186, I get the impression you're doing that querying as part of a module, if so, and this is just a suggestion, but have you tried the Entity Query API, instead of directly querying the database? As that's a much more platform agnostic approach, since it abstracts away a lot of the raw database querying, and means you don't have to worry about the specific table structure that any given site might be using. Definitely the preferred option, especially if you're writing a module.

Anyway, this issue definitely needs an update, the last patch is quite out of date at this point.

The first thing I would think is, how does this impact the Workflow and Content Moderation modules? This seems like something those core modules would be very interested in: if I change the state from draft to published, the published date should update.

More generally, this might be a good candidate for providing a Interface and accompanying Trait in the Entity API, then applying that to the node entity type. That would be very relevant for going along with #2833378: Create an EntityCreatedInterface with accompanying trait to promote code reuse and standardization.

@AaronMcHale:

if I change the state from draft to published, the published date should update

But only the first time that state change occurs? If you unpublish then republish, should the published date change? That's probably business-logic specific. What should happen when content has been unpublished, gone through multiple revisions, and then finally been republished? How does that compare to content that stays published, has one or more drafts on top of that published revision, then an approved draft gets published?

To support an editorial revisioning/publishing workflow, maybe what we really need here is a first_published and last_published timestamp? The former might be a pure core responsibility, bur the latter a workflow/moderation module responsibility (including core workflow/moderation modules), but extendable by contrib workflow/moderation modules.

This should be rescoped to apply to any entity type that uses EditorialContentEntityBase.

#188:

To support an editorial revisioning/publishing workflow, maybe what we really need here is a first_published and last_published timestamp?

If the published_date field is revisionable, then that gives us sufficient storage for all possible use cases and we don't need the 2 fields. For example, normally the publication_date would simply be copied unchanged from the previous revision when creating a revision, and therefore be the original publication date; but if someone wanted to they could make custom logic to set it to null when the entity is unpublished and reset it when the entity is republished.

If publication_date was revisionable, we could have 2 methods:
- getPublicationDate() which gets the publication date from the current revision.
- getOriginalPublicationDate() which queries the entities revisions and gets the publication date of the revision with the earliest publication date

That sounds near-ideal, though what if the entity is revisionable but some client-imposed business logic requires revisions are not created yet they still want to publish/unpublish and track both dates? Don't laugh...

though what if the entity is revisionable but some client-imposed business logic requires revisions are not created yet they still want to publish/unpublish and track both dates?

We only need to consider the 80% use case. For everything else, there's custom fields.

Re @John Pitcairn in #188: Those are all good questions that should be considered. Unfortunately I don't have any answers to those right now. One possible option that comes to mind would be for Content Moderation, providing configuration that allows setting how/when this field should be updated, e.g. configuration could allow choosing on which transition(s) the published date should be updated and if it should only be updated if it was not previously set. I guess that option could even just be done by the Node module itself for each content type, because even if the site doesn't have Content Moderation installed, Node still provides a basic published and unpublish option.

Having said that, I think this issue should just focus on getting the Entity API side of things implemented, then have a follow-up for Node/Workflow/CM specific aspects and interactions.

Re @joachim in #189: Yeah good point, that would be an interesting update hook. It would have to find all of the Entity Types installed on a site and update them appropriately. This is definitely something then that we should look to have as a Trait and Interface, which is included by EditorialContentEntityBase. I wonder if it should be part of or separate from #2833378: Create an EntityCreatedInterface with accompanying trait to promote code reuse and standardization, my gut feeling is separate since that's doing something slightly different.

Re @jonathanshaw #190: That sounds like a sensible solution, if the Entity Type supports revisions, then make it a revisionable field.