Problem/Motivation

In #3255504: Remove jQuery dependency from date.js, @alexpott wrote:

Looking at #3202818: Remove untrue declaration of Opera Mini (in reverse proxy mode) support from Drupal 9.3 we don't support Opera Mini - so I think we can file a follow-up issue to completely remove the library in Drupal 10.0.x and deprecate it in Drupal 9.4.x

See that issue for more details. Deprecate the library.

API changes

core/drupal.date is deprecated. No replacement is needed.

Release notes snippet

JavaScript library core/drupal.date is deprecated. No replacement is needed because no supported browsers require it.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cilefen created an issue. See original summary.

cilefen’s picture

Status: Active » Needs review
FileSize
429 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 3256549-2.patch, failed testing. View results

cilefen’s picture

There is a usage in in Drupal\Core\Render\Element\Date. I am not sure if removing the accompanying data-drupal-date-format DOM attribute would be part of this.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.13 KB

Here it is with the usage removed.

hmendes’s picture

Status: Needs review » Reviewed & tested by the community

Changing this to RTBC as it deprecated the library.
Note: Looks like the usage in Drupal\Core\Render\Element\Date was actually introduced together with the drupal.date library in #1835016: Polyfill date input type

catch’s picture

Status: Reviewed & tested by the community » Needs work

I think we should be removing the supporting markup here too.

cilefen’s picture

I think we’ll need a change record also, yes?

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
cilefen’s picture

FileSize
1.68 KB
cilefen’s picture

FileSize
1.69 KB

The pandemic has been hard on everyone... I think I fixed all my typos and I'm actually following the proper format now.

Status: Needs review » Needs work

The last submitted patch, 11: 3256549-11.patch, failed testing. View results

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.96 KB

Removed a callback usage.

Spokje’s picture

Status: Needs review » Needs work

Looking good, just 2 remarks:

  1. +++ b/core/lib/Drupal/Core/Render/Element/Date.php
    @@ -58,14 +57,14 @@ public function getInfo() {
        */
       public static function processDate(&$element, FormStateInterface $form_state, &$complete_form) {
    -    // Attach JS support for the date field, if we can determine which date
    -    // format should be used.
    -    if ($element['#attributes']['type'] == 'date' && !empty($element['#date_date_format'])) {
    -      $element['#attached']['library'][] = 'core/drupal.date';
    -      $element['#attributes']['data-drupal-date-format'] = [$element['#date_date_format']];
    -    }
    +    @trigger_error('Drupal\Core\Render\Element\Date::processDate() is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. There is no replacement. See https://www.drupal.org/node/3258267', E_USER_DEPRECATED);
         return $element;
       }
    

    Since we removed the usage of this method in getInfo(), I believe the proper way of deprecating this is by simply adding the @trigger_error as the first line and leave the rest intact.

  2. I think we're missing a test that the deprecation message of processDate() is triggered. Since there's no existing test of this method, I believe a new Class calling this method is needed with a method calling processDate() and the appropriate $this->expectDeprecation() just before that line.

The pandemic has been hard on everyone...

Covid related test-failures are the hardest ones ;)

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
2.89 KB

I will be surprised if this passes every coding standard test.

Spokje’s picture

Status: Needs review » Needs work

I will be surprised if this passes every coding standard test.

TestBot agrees with the above :(

cilefen’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
1.03 KB

As expected...

alexpott’s picture

+++ b/core/lib/Drupal/Core/Render/Element/Date.php
@@ -34,7 +34,6 @@ public function getInfo() {
-        [$class, 'processDate'],

Don't we still need to add the library in Drupal 9 - to maintain support there? I agree we can remove the library in Drupal 10 but I think this might be once of those instances where deprecation doesn't really work. Or is the case the even in Drupal 9.4 there are no supported browsers that need this library? I thought ie support is only being dropped in Drupal 10.

cilefen’s picture

Or is the case the even in Drupal 9.4 there are no supported browsers that need this library? I thought ie support is only being dropped in Drupal 10.

I think no. #3202818: Remove untrue declaration of Opera Mini (in reverse proxy mode) support from Drupal 9.3 seems just to have removed Opera-mini compatibility in 9 rather than deprecating anything.

cilefen’s picture

I used to like all this tooling.

cilefen’s picture

Or is the case the even in Drupal 9.4 there are no supported browsers that need this library?

I have no idea on that point.

cilefen’s picture

cilefen’s picture

Status: Needs review » Needs work

So IE needs it and yes, there really isn't a way to deprecate this. I suppose it should simply be removed in 10?

catch’s picture

We could deprecate it, but then suppress the deprecation notice in the listener, then it would be documented but wouldn't actually trigger deprecation notices. We never do that for PHP deprecations any more (or try not to at least), but maybe for a library like this it'd be fine.

longwave’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
527 bytes

So we have to leave ::processDate in place for now, let's see how many tests this blows up.

Status: Needs review » Needs work

The last submitted patch, 25: 3256549-25.patch, failed testing. View results

murilohp’s picture

Status: Needs work » Needs review
FileSize
3.77 KB
1.73 KB

Just following your suggestion here @catch, let's see the tests now.

murilohp’s picture

Fixing CS here

catch’s picture

#28 looks OK to me, the only other option with ::processDate() would be to mark it @internal, for removal in Drupal 10, but I don't think it hurts to have @deprecated on there and suppress it.

We need a 10.x patch with the usage/library etc. all removed too.

murilohp’s picture

Great! Here's a D10 only patch.

murilohp’s picture

Thinking about it here, I think we need to merge #28 on D10 first @catch, this way for D10 patch, I can remove the deprecated test class and change the deprecation listener trait, what do you think?

catch’s picture

Status: Needs review » Reviewed & tested by the community

@murilohp it's usually easier to have completely separate Drupal 10 and Drupal 9 patches, which can be committed at the same time (in the window where we're still committing deprecations to Drupal 9 and removing them from Drupal 10, otherwise we have to follow the workflow you suggested). In which case #28 and #30 both look good to me.

Actually let's mark this RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2fc40e8 and pushed to 10.0.x. Thanks!
Committed baa0ed6 and pushed to 9.4.x. Thanks!

  • alexpott committed 2fc40e8 on 10.0.x
    Issue #3256549 by cilefen, murilohp, longwave, catch, Spokje: Remove...

  • alexpott committed baa0ed6 on 9.4.x
    Issue #3256549 by cilefen, murilohp, longwave, catch, Spokje: Deprecate...

Status: Fixed » Closed (fixed)

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