Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
#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.
Comment | File | Size | Author |
---|---|---|---|
#38 | 2006484-38--correct_comment.patch | 389 bytes | drunken monkey |
#31 | borked_datetime_format.png | 6.76 KB | Pancho |
#31 | interdiff_28_31.txt | 2.91 KB | Pancho |
#31 | 2006484-second_followup-31.patch | 3.41 KB | Pancho |
#28 | 2006484-second_followup-28.patch | 3.24 KB | Pancho |
Comments
Comment #1
tim.plunkettThis 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?
Comment #2
dawehnerIs it worth to explain why this update is empty? Why not just remove it totally?
Comment #3
tim.plunkettThere is a precedent for not removing update hooks. I tweaked this one to mimic image_update_8001(), which is also empty.
Comment #7
tim.plunkettWell, crap. I hate mucking with testModuleEnableOrder().
Maybe I should kill comment's dependency on datetime too...
Comment #11
tim.plunkettMissed one part change in the dependency test.
Comment #12
dawehnerThis feels more sane.
Comment #13
tim.plunkettReroll for Drupal::state() and update_module_enable()
Comment #18
dawehnerCan and should be typehint here?
Comment #19
tim.plunkettYes! admin/content added a NodeBCDecorator, so its safe to typehint.
Comment #20
dawehnerCool. Thank you very much
Comment #22
tim.plunkett#19: datetime-2006484-19.patch queued for re-testing.
Comment #23
tim.plunkettfailed to checkout from [git://git.drupal.org/project/drupal.git]
Comment #24
alexpottCommitted 9806049 and pushed to 8.x. Thanks!
Comment #25
amateescu CreditAttribution: amateescu commentedThis patch introduced two (identical) notices on the node edit page:
While I was there, I also had to fix
theme_datetime_wrapper()
to actually use the description provided bydatetime_form_node_form_alter()
. Attached the before/after screenshots.Before:
After:
Comment #26
amateescu CreditAttribution: amateescu commentedClosed a duplicate that was fixing the same thing.. #2019843: Notice: Array to string conversion in datetime_form_node_form_alter() (line 1132 of core\modules\datetime\datetime.module)..
Comment #27
alexpottCommitted 033451a and pushed to 8.x. Thanks!
Comment #28
PanchoThis 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
Subsequently, editing nodes doesn't work anymore, therefore critical.
datetime_form_node_form_alter() did a:
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.
Comment #29
PanchoForgot to add the screenshot. Here it is.
Comment #30
alexpottIt appears that $date_format would not be defined at this point...
Comment #31
PanchoThank 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:
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.
Comment #32
PanchoComment #33
tim.plunkettComment #34
ParisLiakos CreditAttribution: ParisLiakos commentedNext time plz open new issues
Comment #35
Pancho@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.
Comment #36
dawehnerIsn'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.
Comment #37
PanchoSeems 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.
Comment #38
drunken monkeyIt 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 usegit blame
to find this one.Patch attached for fixing this, unless it is somehow on purpose.
Comment #39
tstoecklerYeah, that makes sense.
Comment #40
catchCommitted/pushed the follow-up.