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.
Problem/Motivation
The date type element has #date_date_callback option which can be used to alter it.
But it doesn't work properly because of function_exists() function inside processDatetime method of Datetime element. It always returns FALSE for all callbacks added in the #date_date_callback option.
Proposed resolution
Repace function_exists() by call_user_func_array() to be able execute callback with context
Comment | File | Size | Author |
---|---|---|---|
#9 | 2992580-09.patch | 3.3 KB | Vj |
#9 | 2992580-09-testonly.patch | 2.61 KB | Vj |
Comments
Comment #2
Brolad CreditAttribution: Brolad at Skilld commentedComment #3
jhedstromThanks for this! I've noticed myself that callables cannot be used here, and only functions, which is really annoying.
I think we still need to check if this is valid, so replacing
function_exists
withis_callable
, and then usingcall_user_func_array
should work.Comment #4
andypostit needs tests and surely must use
is_callable
instead offunction_exists
Comment #5
Vj CreditAttribution: Vj as a volunteer commentedTested below patch with following datetime field
@andypost : Can you give some idea about test to write, should i write tests in core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php file ? and what can be added to compare.
Comment #6
andypost@Vj I suppose there should be tests for
datetime
form element (very probably it does not exists for core/lib/Drupal/Core/Datetime/Element/Datetime.php)Example is core/tests/Drupal/KernelTests/Core/Element/PathElementFormTest.php but I can't find similar one for this element
Comment #7
Vj CreditAttribution: Vj as a volunteer commented@andypost
Tried creating test for the first time :P
Please review and let me know if it works or any changes required.
Comment #8
andypost@Vj it looks great! Please attach patch with test only to show that without fix it will fail
Comment #9
Vj CreditAttribution: Vj as a volunteer commentedtestonly patch should fail
Comment #11
jhedstromThis looks great. Thanks all!
Comment #12
catchFixed these code style issues locally, and committed/pushed to 8.7.x, thanks!
Comment #15
dwwCan we fix this in 8.6.x, too?
Patch applies cleanly.
Comment #16
dwwAs expected, bot is happy on 8.6.x.
Cherry-picking this to 8.6.x would unblock #2672950: Notice: Undefined index: #default_value in Drupal\Core\Datetime\Element\Datetime::valueCallback() (line 103 which in turn unblocks / simplifies #2419131: [PP-1] #states attribute does not work on #type datetime.
This seems like an utterly non-disruptive bugfix with test coverage.
RTBC.
Thanks!
-Derek
Comment #17
dwwSorry, in case that wasn't clear, I had re-triggered patch #9 to test against 8.6.x and it's all green...
Thanks,
-Derek
Comment #18
Wim LeersThis indeed looks very non-disruptive.
Comment #19
alexpottI agree we can backport this.