Deprecate the AJAX RenderElement

Seems like a relic from Drupal 7.

Nice find!

Unless this is broken code this needs to go through the deprecation process as this is a plugin and potentially could be being used by contrib or custom code. Deprecating would mean overriding the constructor and throwing an @trigger_error() in there. And adding a @deprecated doc tag to the class docs.

Thanks! I've found one small moment which need to fix. In Drupal coding standards is recomendation about parametrs order in docblock:

https://www.drupal.org/project/drupal/issues/3067580 section "Drupal API documentation standards for order of documentation sections"

Would be good apply it to the patch.

Does anyone know how to make use of this element in Drupal 8? If this is a dead code from Drupal 7 there is no need in deprecation process and change record.

1748 failed tests means it used somehow

As #3 shows, removing it passes tests.
The problem is that on a cold cache, all RenderElement plugins are instantiated and their getInfo() method is called.
I'm not sure how best to indicate deprecations of these plugins... Maybe a #deprecated = TRUE flag or something.

Nice find indeed!

For the record, the last usage of this was removed in #1957590: Remove remaining procedural ajax command usages., April 2013. 381 commits before 8.0-alpha1!

😮

Nice git archeology work!

Added initial CR https://www.drupal.org/node/3068104

Patch introduce "deprecated" annotation for elements plugins as #14 suggested and fixed deprecation message

+++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
@@ -131,6 +131,17 @@ protected function buildInfo($theme_name) {
+  /**
+   * {@inheritdoc}
+   */
+  protected function findDefinitions() {
+    $definitions = parent::findDefinitions();
+    // Filter out definitions that deprecated.
+    return array_filter($definitions, function ($definition) {
+      return !empty($definition['deprecated']);
+    });
+  }

This doesn't work - we need deprecated definitions to be findable.

Here's a patch based on a comment from @andypost online.

We need to add tests how this works but it should allow us to build element info and use the current plugin deprecation standard (i.e. @trigger_error in the constructor).

Here's a test/

Fixed up the deprecation message. We can add an @see if we feel a change record is warranted. @tim.plunkett what would a CR say?

drupal:8.8.0

Is this a new format for referencing Drupal versions in deprecation messages? It used to be "Drupal 8.8.0".

Oops there is a CR already. Nice. Added it to the deprecations. @Chi yes this is the format.

Hmmm... so I've tested whether or not the Ajax element works. And it doesn't. It is totally busted. I set up a controller to do:

  public function testAjax() {
    $commands[] = new ReplaceCommand('#edit-date-format-suffix','<small id="edit-date-format-suffix">A test</small>');
    return ['#type' => 'ajax', '#commands' => $commands];
  }

And the only JS loaded on the page is

<!--[if lte IE 8]>
<script src="/core/assets/vendor/html5shiv/html5shiv.min.js?v=3.7.3"></script>
<![endif]-->

So I ponder if the CR is even worth it and if removing it isn't actually the right thing.

This element does not work, so probably removal is better choice

+++ b/core/lib/Drupal/Core/Render/Element/Ajax.php
@@ -10,9 +10,22 @@
+    @trigger_error('\Drupal\Core\Render\Element\Ajax is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Return an \Drupal\Core\Ajax\AjaxResponse instead. See https://www.drupal.org/node/3068104', E_USER_DEPRECATED);

+++ b/core/modules/system/tests/modules/element_info_test/src/Element/Deprecated.php
@@ -0,0 +1,29 @@
+    @trigger_error(__NAMESPACE__ . '\Deprecated is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. See https://www.example.com', E_USER_DEPRECATED);

Could use __CLASS__

Addressed the comment #24
Attached patch against Drupal 10.1.x

Deprecation messages also needs update to 10.1 and 11.0 from 8 and 9

Fixed failed commands on #32
Addressed the comment #33
Attached interdiff patch

Here's a fix, also it needs extra test case for the deprecated Ajax class

Maybe extra CR needed to announce ability to deprecate elements

+++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
@@ -106,6 +106,16 @@ protected function buildInfo($theme_name) {
+    $previous_error_handler = set_error_handler(function ($severity, $message, $file, $line, $context) use (&$previous_error_handler) {
...
+        return $previous_error_handler($severity, $message, $file, $line, $context);

since PHP 8.0 $context is removed https://www.php.net/manual/en/function.set-error-handler.php that's why

2) Drupal\KernelTests\Core\Asset\AttachedAssetsTest::testAddExternalFiles
ArgumentCountError: Too few arguments to function Drupal\Core\Render\ElementInfoManager::Drupal\Core\Render\{closure}(), 4 passed and exactly 5 expected

and will need postponed issue to remove the element

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Reviewing #37

Deprecation message appears correct
Applies cleanly locally.
Verified test fails and passes with the deprecation fix.
CR looks good.

+1 for me

Unless you meant in #38 this should be postponed?

+++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
@@ -107,6 +107,16 @@ protected function buildInfo($theme_name) {
     $info = [];
+    $previous_error_handler = set_error_handler(function ($severity, $message, $file, $line) use (&$previous_error_handler) {
+      // Ignore deprecations while building element information.
+      if ($severity === E_USER_DEPRECATED) {
+        // Don't execute PHP internal error handler.
+        return TRUE;
+      }
+      if ($previous_error_handler) {
+        return $previous_error_handler($severity, $message, $file, $line);
+      }
+    });

Shame we have to do this but the only way around it would seem to be moving ::getInfo() to attributes or something. Not for here.

Everything looks good to me. I don't think we need an explicit postponed issue to remove this, we can do it as part of the general 'remove deprecated stuff' Drupal 11 issues.

Committed d4da802 and pushed to 10.1.x. Thanks!

We could keep that code and the test for it there - so contrib can deprecate render elements too.

Alternatively we could deprecate not having getInfo() as a static method - or we could even try to move getInfo() into PHP attributes somehow - although things like the date format loading in \Drupal\Core\Datetime\Element\Datetime::getInfo() are likely to be tricky but we could have an alter or something similar for that.