Add #optional support to the Container render element

Ok clearly i got too excited. Let's try this again...

Thanks @amateescu for the review, all good points.

Here is a patch addressing #5 and adding a WIP render test.

I believe the test cases are ok, but the test itself is not correct... I only seem to get the result of rendering what's inside the container, but not the wrapping divs around it, any clues how to do this properly? Unfortunately there is no test coverage for Details::preRenderDetails to have a look at.

Alright I can't figure that one out, so taking a different route now by defining a new BrowserTest instead.

I'm getting a weird InvalidArgumentException: Invalid database prefix: in Drupal\Core\Test\TestDatabase->__construct() (line 81 of /var/www/drupal8/core/lib/Drupal/Core/Test/TestDatabase.php). error locally on all tests making use of form_test module, hoping it's just me, let's see what the bot says...

OK at least its only me with the errors. Hopefuly this will come back green.

Any clue why somone would get this error?

$ sudo -u www-data PHP_IDE_CONFIG="serverName=drupal8" SIMPLETEST_DB=mysql://admin:admin@localhost/drupal8_tests SIMPLETEST_BASE_URL=http://drupal8 ../vendor/bin/phpunit -v /var/www/drupal8/core/modules/system/tests/src/Functional/Form/ElementsContainerTest.php
[sudo] password for manuel: 
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Runtime:	PHP 7.0.18-0ubuntu0.16.04.1 with Xdebug 2.4.0
Configuration:	/var/www/drupal8/core/phpunit.xml.dist

Testing Drupal\Tests\system\Functional\Form\ElementsContainerTest
E

Time: 12.32 seconds, Memory: 6.00MB

There was 1 error:

1) Drupal\Tests\system\Functional\Form\ElementsContainerTest::testOptionalContainerElements
ReflectionException: Class Drupal\form_test\EventSubscriber\FormTestEventSubscriber does not exist

/var/www/drupal8/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterEventSubscribersPass.php:30
/var/www/drupal8/vendor/symfony/dependency-injection/Compiler/Compiler.php:120
/var/www/drupal8/vendor/symfony/dependency-injection/ContainerBuilder.php:573
/var/www/drupal8/core/lib/Drupal/Core/DrupalKernel.php:1299
/var/www/drupal8/core/lib/Drupal/Core/DrupalKernel.php:883
/var/www/drupal8/core/lib/Drupal/Core/DrupalKernel.php:797
/var/www/drupal8/core/lib/Drupal/Core/Extension/ModuleInstaller.php:544
/var/www/drupal8/core/lib/Drupal/Core/Extension/ModuleInstaller.php:195
/var/www/drupal8/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/var/www/drupal8/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:419
/var/www/drupal8/core/tests/Drupal/Tests/BrowserTestBase.php:978
/var/www/drupal8/core/tests/Drupal/Tests/BrowserTestBase.php:443

FAILURES!
Tests: 1, Assertions: 0, Errors: 1.

OK this is embarrassing... let's try it again.

OK the failure seems correct, so it looks like we need to have a look at preRenderContainer.

That said I cannot work on this until I can actually run the test locally (see #12), so essentially I'm blocked.

OK did a bit of thinking, and decided to try this, Container is not the same as Details, so I think doing it like this makes more sense since you can just add child elements directly under the container in the array.

Also, please keep in mind that I am still unable to run this test locally, so I have to rely on the bot's feedback :)

Perhaps #17 wasn't such a good idea... can't really do much else here sorry, but we should continue from #14.

OH I get it... also had a look at Element::isVisibleElement, which helped finally clear this out for me.

So a visible element has nothing to do with whether or not when you render the element it actually produces any markup. A visible element is just an element who's #type is not hidden, value nor token, and that if it has an #access property, it is (or returns) TRUE.

Thanks so much @amateescu for helping out on this, much appreciated!

I think we're good to go with this now :D

Thanks @dawehner for the review!
#22.1 Done
#22.2 All makes sense to me, Code changes done, change record coming soon.
#22.3 Done
#22.4 Don't see why not, done.

Missing comma... oops!

Added CR: https://www.drupal.org/node/2895127 :)

It's good to have tests. Fixing logic introduced in #23...

Thanks @vijaycs85 for the review!
Re #28:

1. I forget comments are for humans - good call, done.
2. Checking #printed prevents unneeded calls to Element::getVisibleChildren(), so we should do that in Details as well. Probably doesn't occur too often but every little bit counts I suppose. Either way, not in scope here =)
3. I suppose that makes the test easier to understand, done.
4. Thanks =)

Yeah I guess the changes to Actions is of out of scope, and probably very rarely this would have a use-case there. We can always add it later if people want it. Removing those changes here.

I also attach a patch which we could use should we ever want to add the optional property to Actions.

Fixing also #30.2 here.

Updated the CR removing references to the Actions element.
https://www.drupal.org/node/2895127

Ping...

Sorry to bring this up, but it is sort of blocking #2892304: Introduce footer region to ContentEntityForm, which in turn is making us repeat unnecessary code for published check-boxes (like on #2834546: UI for publishing/unpublishing block_content blocks)... :S

Apologies @lauriii - I should've updated the description here after #21, thanks for the extra effort in understanding the issue!

Yup details probably suffers from the same bug... should we be patching Element::isVisibleElement to solve it globally? Not sure if that would tackle the problem with the themes that wim mentions. Either way, I don't think we should be fixing this here.