Move views-element-container to Twig template

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Re-rolled #52

That todo may need to be added back. Was the plan to get in for D8 and removed in D9? Would the plan be the same just different versions?

Can't tell if the issue summary update ever happened but it was tagged for that + change record.

Added back * @todo to be removed altogether in 9.x to #58

Still needs an issue summary and #58 was an open question not be implemented. Why would we remove in 9 when 9 is EOL?

@#44 thanks for finally one report back then :-) much appreciated. after my last patches a lot of time has gone so I am happy to see more progress on it lately again. Thanks for all the work on it!

Issue summary updated.

And here are a couple more things for context:

1. This problem isn't limited to blocks and EVA, but literally any time ::buildRenderable() is called, such as this custom code:
   

   $view_id = 'someview';
   $view_display = 'block_1';
   if ($view = Views::getView($view_id)) {
     $view->setDisplay($view_display);
     $view->execute();
     $results = $view->buildRenderable($view_display);
   }
   

   The Site Studio Views Element → module is also affected.

2. There is an issue that predates this one, whereby someone had done some investigation that identified one possible use case for this wrapper having to do with #states api to conditionally hide/show form elements based on presence of a view.

   See comment #18 here: #2575405-18: Is the

   with class "views-element-container" around views pages needed? → .

   I'm not 100% certain where this #states api use case came from or whether core or contrib uses it in any way, or if that use case is purely speculative.

Therefore, I suggest we:

• Consider closing this issue as a duplicate of #2575405, and moving latest patches, and issue credit to the older issue.
• Combine forces and refocus the effort around first moving the div+class out of a preRender into a Twig template where it belongs because A) theming this to override and remove it from the preRender is really hard, and B) HTML DOM and classes are supposed to be in templates as a best practice since Drupal 8.
• Then, determine if the #states api use case is a valid use-case, because it will dictate whether we need to introduce a new TEST that actually leverages the #states api to prove that moving this to a Twig template doesn't break that functionality. Sorry. :(
• Finally, discuss whether we can mark the template and functionality as deprecated in a follow-up commit and remove it in next major version (Drupal 11 at this point) with an appropriate change record, eg to show how code that leverages #states api can be reworked (if it is determined that this is a real use case).

I've updated the 9.x to 11.x to address #60, however... since Drupal dev branch naming has changed, the @todo as written is no longer intuitive, because 11.x branch is where Drupal 10 development is taking place. ¯\_(ツ)_/¯

Maybe it should say:

* @todo to be removed altogether in next major version

I decided to go with:

 * @todo Remove as part of https://www.drupal.org/node/2721899

To avoid more needless rerolls in the future ;)

Grammar fix issue title

Re #63:

discuss whether we can mark the template and functionality as deprecated and remove it in next major version with an appropriate change record, eg to show how code that leverages #states api can be reworked (if it is determined that this is a real use case)

.

According to comment #52 on the other issue #2575405-52: Is the

Patch update to address #67.

Patch in #68 (incorrectly named as patch number 69) was to address #67. Leaving a comment here to justify it.

+++ b/core/modules/views/tests/src/Functional/ViewElementTest.php
@@ -40,8 +40,6 @@ public function testViewElement() {
-    // Verify that the view container has been found on the form.
-    $this->assertSession()->elementExists('xpath', '//div[@class="views-element-container js-form-wrapper form-wrapper"]');

@@ -80,8 +78,6 @@ public function testViewElementEmbed() {
-    // Verify that the view container has been found on the form.
-    $this->assertSession()->elementExists('xpath', '//div[@class="views-element-container js-form-wrapper form-wrapper"]');

If these changes to existing tests are needed, that seems to imply that we are changing the output with the current fix, while the whole idea of doing it this way was that existing sites would not be affected. Are these test changes really needed, and if so, I'm wondering why.

Excellent point.

Let's find out.

The last submitted patch, 72: 2721899-72.patch, failed testing. View results →

1.- It looks like tests are not passing so this needs more work.

2.-

Are these test changes really needed, and if so, I'm wondering why.

IMO, yes, as stated in issue summary:

Since Drupal 8, classes and divs should preferentially be added in Twig templates when possible, but because this div and class are added in a preRender function, it makes things very difficult to override and/or remove.

The only known fix for this is via a long-standing, somewhat hacky workaround from @idiaz.roncero:

https://gist.github.com/idiazroncero/34425dbed9df73a44046252582ac1689

Copy the following into YOURTHEME/templates/form/container.html.twig

{%
  set classes = [
    has_parent ? 'js-form-wrapper',
    has_parent ? 'form-wrapper',
  ]
%}

{# Remove unnecessary <div class="views-element-container"> wrapper #}
{% if attributes.hasClass('views-element-container') %}
  {{ children }}
{% else %}
  <div{{ attributes.addClass(classes) }}></div>{{ children }}</div>
{% endif %}

Hacky because writing one-off exceptions in Twig like that is not really an intuitive approach.