Problem/Motivation

The #date_date_callbacks expects $element to be passed by reference but the change in #3213572: #date_time_callbacks and #date_date_callbacks bypass the TrustedCallbackInterface protections has broken this.

Steps to reproduce

Run webform tests on Drupal 9.3.x - or anything that uses #date_date_callbacks. Unfortunately core has no real usage only a test usage.

See https://git.drupalcode.org/project/webform/-/blob/6.x/includes/webform.d... for an existing callback function that is broken on 9.3.x.

Proposed resolution

Add an & to StaticTrustedCallbackHelper::callback($callback, [$element, $form_state, $date], $message, TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION);

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3250335-6.patch6.08 KBalexpott
#6 4-6-interdiff.txt4.51 KBalexpott
#4 3250335-3.patch5.17 KBalexpott
#4 3250335-3.test-only.patch3 KBalexpott

Issue fork drupal-3250335

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#3213572: #date_time_callbacks and #date_date_callbacks bypass the TrustedCallbackInterface protections
beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented
Assigned: Unassigned » beatrizrodrigues
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3250335-3.test-only.patch3 KB
new 3250335-3.patch5.17 KB

Here's the fix.

Here's an example of an existing callback that is broken by this: https://git.drupalcode.org/project/webform/-/blob/6.x/includes/webform.d...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -274,7 +274,7 @@ public static function processDatetime(&$element, FormStateInterface $form_state
-          StaticTrustedCallbackHelper::callback($callback, [$element, $form_state, $date], $message, TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION);
+          StaticTrustedCallbackHelper::callback($callback, [&$element, &$form_state, &$date], $message, TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION);

I'm not sure if we need the & on the $form_state and $date because they are objects but also arrays are copied so...

Gonna try to work out how to test this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 4-6-interdiff.txt4.51 KB
new 3250335-6.patch6.08 KB

Nope we don't need &'s for the objects - it's just the arrays that are being copied. Here's tests for that.

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented
Assigned: beatrizrodrigues » Unassigned
Status: Needs review » Active
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Active » Needs review

The last submitted patch, 4: 3250335-3.test-only.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

I reran the updated test locally without the fix to the Datetime element and it fails as expected. The fix itself is trivial, the updated test looks good to me as well.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

RTBC +1

Before patch is applied i got the error when running webform test WebformElementDateTimeTest

1) Drupal\Tests\webform\Functional\Element\WebformElementDateTimeTest::testDateTime
Exception: Warning: Parameter 1 to _webform_datetime_date() expected to be a reference, value given
Drupal\Core\Security\StaticTrustedCallbackHelper->doTrustedCallback()() (Line: 101)

After patch is applied, the test passes.

The code looks good as well.

  • catch committed 1a8a51a on 9.4.x 
    Issue #3250335 by alexpott, beatrizrodrigues, paulocs, longwave: #...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.4.x and cherry-picked to 9.2.x, thanks!

  • catch committed 3f5645f on 9.3.x 
    Issue #3250335 by alexpott, beatrizrodrigues, paulocs, longwave: #...

Status: Fixed » Closed (fixed)

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