#501428: Date and time field type in core added datetime as a hard dependency of node.module to improve the "Authored on" field.

In addition to being unnecessarily tight coupling, since the testing profile always enabled node.module, datetime is added to every test run.

Many modules might want to improve the node form, datetime can serve as an example of how to do that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
4.05 KB

This restores NodeFormController to its original state, and uses hook_form_alter() and hook_node_prepare() to make the exact same changes.

If you use standard.profile, you will still see the awesome new datetime picker.

Instead of deleting, I emptied out the update hook. I think that's the standard practice?

dawehner’s picture

+++ b/core/modules/node/node.installundefined
@@ -808,11 +808,9 @@ function node_update_8013() {
+ * Empty update.

Is it worth to explain why this update is empty? Why not just remove it totally?

tim.plunkett’s picture

FileSize
352 bytes
4.09 KB

There is a precedent for not removing update hooks. I tweaked this one to mimic image_update_8001(), which is also empty.

tim.plunkett’s picture

Well, crap. I hate mucking with testModuleEnableOrder().
Maybe I should kill comment's dependency on datetime too...

tim.plunkett’s picture

FileSize
7.36 KB
841 bytes

Missed one part change in the dependency test.

dawehner’s picture

FileSize
934 bytes
7.38 KB

This feels more sane.

tim.plunkett’s picture

FileSize
7.39 KB

Reroll for Drupal::state() and update_module_enable()

dawehner’s picture

+++ b/core/modules/datetime/datetime.moduleundefined
@@ -1120,3 +1120,23 @@ function datetime_range_years($string, $date = NULL) {
+function datetime_node_prepare($node) {

Can and should be typehint here?

tim.plunkett’s picture

FileSize
780 bytes
7.62 KB

Yes! admin/content added a NodeBCDecorator, so its safe to typehint.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Cool. Thank you very much

Status: Reviewed & tested by the community » Needs work
Issue tags: -Datein8, -Test suite performance

The last submitted patch, datetime-2006484-19.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Datein8, +Test suite performance

#19: datetime-2006484-19.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

failed to checkout from [git://git.drupal.org/project/drupal.git]

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9806049 and pushed to 8.x. Thanks!

amateescu’s picture

Status: Fixed » Needs review
FileSize
13.96 KB
11.09 KB
1.31 KB

This patch introduced two (identical) notices on the node edit page:

Notice: Array to string conversion in datetime_form_node_form_alter() (line 1132 of core\modules\datetime\datetime.module).

While I was there, I also had to fix theme_datetime_wrapper() to actually use the description provided by datetime_form_node_form_alter(). Attached the before/after screenshots.

Before:
author-date-before.png

After:
author-date-after.png

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 033451a and pushed to 8.x. Thanks!

Pancho’s picture

This doesn't seem to work with DateTime disabled and intl being available.

In this case, the custom, hard-coded formatter completely borks format_date(), so when editing any node I 'm getting now a

Warning: date_format() expects parameter 1 to be DateTime, boolean given in Drupal\node\NodeFormController->form() (line 195 of core/modules/node/lib/Drupal/node/NodeFormController.php).

Subsequently, editing nodes doesn't work anymore, therefore critical.

datetime_form_node_form_alter() did a:

$config = Drupal::config('system.date');
$format = $config->get('formats.html_date') . ' ' . $config->get('formats.html_time');

and we have to do that just as well, not only as a DateTime form alter, but in any case.

Here's a very quick fix. Seemed to work, but I'm in a hurry, so can't do more testing now.

Note that datetime_default_format_type() despite its name lives in common.inc, so we still not need Datetime.

Pancho’s picture

Priority: Normal » Critical
Status: Fixed » Needs review
FileSize
86.94 KB

Forgot to add the screenshot. Here it is.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/datetime/datetime.moduleundefined
@@ -1137,13 +1137,9 @@ function datetime_range_years($string, $date = NULL) {
+  $form['author']['date']['#description'] = t('Format: %format. Leave blank to use the time of form submission.', array('%format' => datetime_format_example($date_format)));

It appears that $date_format would not be defined at this point...

Pancho’s picture

Status: Needs work » Needs review
FileSize
3.41 KB
2.91 KB
6.76 KB

Thank you, that's true.
Took another approach making the field a real 'datetime' field by merging in the defaults, so we can use datetime_html5_format().

However, with Datetime enabled, this still doesn't work, see new screenshot with Datetime enabled:
Screenshot
Also note, that the field itself should be prefilled with the current value.
It seems, there's more wrong, but that's not bound to my patch. It was wrong before. Unfortunately I have no time for investigating.

So all this patch does is making it work again in setups without Datetime enabled.

Pancho’s picture

Status: Needs review » Needs work
tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
ParisLiakos’s picture

Title: Remove dependency on datetime from node » [Followup] Remove dependency on datetime from node

Next time plz open new issues

Pancho’s picture

Component: datetime.module » node.module

@ParisLiakos:
Sorry, but it seems quite inconsistent, in which cases a new issue is opened and when not.
IMHO it's correct to stay on the main issue if the followup is critical, so not getting it completely fixed until release would warrant a rollback of the main patch. But I'm fine with any other policy.

Regarding the bug, I'll be looking into this tonight, because it's extremely annoying.
Next step would be #2027959: Remove dependency on datetime from comment.

dawehner’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -42,7 +42,10 @@ protected function prepareEntity() {
-      $node->date = format_date($node->created, 'custom', 'Y-m-d H:i:s O');
+      $config = \Drupal::config('system.date');
+      $format_type = datetime_default_format_type();
+      $format = $config->get('formats.html_date.pattern.' . $format_type) . ' ' . $config->get('formats.html_time.pattern.' . $format_type);
+      $node->date = format_date($node->created, 'custom', $format);

Isn't $node->date just about getting the node create time stored purely for form related functionality. Based on that, I don't see a reason to load the date format.

Pancho’s picture

Title: [Followup] Remove dependency on datetime from node » Remove dependency on datetime from node
Priority: Critical » Normal
Status: Needs work » Fixed

Seems like #2000384: php-intl module causes problems with Date and Time fields when editing Basic Page covers the regression as a followup.
Reverting this issue to the pre-#28 state.

drunken monkey’s picture

Status: Fixed » Needs review
FileSize
389 bytes

It puts me a bit off that no-one else seems to have noticed, but the new comment for node_update_8014() references a completely unrelated issue, #1836392: In the Views UI, the interaction pattern of “All displays”/ “Override this display” is confusing . Had to use git blame to find this one.
Patch attached for fixing this, unless it is somehow on purpose.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, that makes sense.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed the follow-up.

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