Modernize custom_block.module forms

Initial patch attached. One part I wasn't sure about is:

Cache::invalidateTags(array('content' => TRUE));

Looking through core there are a couple of ways to achieve this.

#2: customblockformcontroller-2060865-2.patch queued for re-testing.

Hold on for the t() insertion, #2059245: Add a FormBase class containing useful methods would ideally handle that.

@larowlan, I did look but couldn't find an interface. LanguageManager doesn't implement one and it seems it's used directly elsewhere in core, eg: \Drupal\Core\Datetime\Date

Comment updated.

Holding off on t() as suggested by tim.plunkett.

1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -8,13 +8,66 @@
   +use Symfony\Component\DependencyInjection\ContainerInterface;
   +use Drupal\Core\Cache\Cache;
   +use Symfony\Component\HttpFoundation\Request;
   

   Might as well put these in order

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -8,13 +8,66 @@
   +  protected $storageController;
   ...
   +    $this->storageController = $storage_controller;
   

   Can we call this customBlockStorage instead? We're soon to be ditching the term 'controller', and its more descriptive

3. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -8,13 +8,66 @@
   +   *   The Module Handler.
   ...
   +   *   The Request.
   ...
   +   *   The Language Manager.
   

   The capitalization is weird here.

4. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -8,13 +8,66 @@
   +      $container->get('request'),
   

   You shouldn't inject the request, you can put it at the end of buildForm() and store it there.

5. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -69,11 +122,10 @@ public function form(array $form, array &$form_state) {
   +    $language_configuration = $this->moduleHandler->invoke('language', 'get_default_configuration', array('custom_block', $block->type->value));
   

   language_get_default_configuration() is not a hook. This should just check moduleExists for language, and then call it. This is just an old style hack.

All issues above fixed.

Sorry about the delay, somehow missed this failure.

1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -8,13 +8,62 @@
   +class CustomBlockFormController extends EntityFormControllerNG implements EntityControllerInterface {
   

   Form controllers no longer use EntityControllerInterface

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -8,13 +8,62 @@
   +  /**
   +   * The request.
   +   *
   +   * @var \Symfony\Component\HttpFoundation\Request
   +   */
   +  protected $request;
   
   @@ -37,6 +86,15 @@ protected function prepareEntity() {
   +  public function buildForm(array $form, array &$form_state, Request $request = null) {
   +    $this->request = $request;
   +
   +    return parent::buildForm($form, $form_state);
   

   This is wrong, use $this->getRequest()

3. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -8,13 +8,62 @@
   +    parent::__construct($module_handler);
   ...
   +      $container->get('module_handler'),
   

   Don't inject the module handler anymore

4. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -8,13 +8,62 @@
   +  public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info) {
   

   just use create(ContainerInterface $container) now

You also need to use $this->t() everywhere, and should remove Overrides/Implements for {@inheritdoc}

And please add a row to #2072251: [meta] Modernize forms to use FormBase for this.

Everything in #12 addressed. I replaced Overrides with inheritdoc apart from when there were additional comments.

Parent issue: #2072251: [meta] Modernize forms to use FormBase

CustomBlockDeleteForm::buildForm() still has request in it.

1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -8,13 +8,50 @@
   +  public function __construct(EntityStorageControllerInterface $custom_block_storage, LanguageManager $language_manager) {
   +
   +    $this->customBlockStorage = $custom_block_storage;
   

   Weird extra line here

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -37,14 +74,14 @@ protected function prepareEntity() {
   -      drupal_set_title(t('Edit custom block %label', array('%label' => $block->label())), PASS_THROUGH);
   +      drupal_set_title($this->t('Edit custom block %label', array('%label' => $block->label())), PASS_THROUGH);
        }
   

   Use ['#title'] please

Fixed #14 and a few other calls to t() in CustomBlockDeleteForm.

I also noticed this above the title. // @todo Remove this once https://drupal.org/node/1981644 is in. That issue seems to have been resolved but I don't understand why we'd remove it?

Never mind, just realised the @todo was relative to drupal_set_title() new patch and interdiff attached. Just ignore #15

1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -8,13 +8,49 @@
   +use Drupal\Core\Controller\ControllerInterface;
   ...
   +class CustomBlockFormController extends EntityFormControllerNG implements ControllerInterface {
   

   EntityFormControllerNG already implements ContainerInjectionInterface, you can remove the ControllerInterface bit

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -37,14 +73,13 @@ protected function prepareEntity() {
   -      drupal_set_title(t('Edit custom block %label', array('%label' => $block->label())), PASS_THROUGH);
   +      $form['#title'] = $this->t('Edit custom block %label', array('%label' => $block->label()));
   

   Can we get some screenshots of this in action with some chatacters like '&' in the title? Not sure how the PASS_THROUGH works with this.

3. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -62,23 +97,25 @@ public function form(array $form, array &$form_state) {
   +    if($this->moduleHandler->moduleExists('language')) {
   

   Missing space after if

1. Is fixed, I removed the use statement and another unused one for DateTime.

2. PASS_THROUGH means that drupal_set_title() just leaves the passed input alone so the before and after are identical.

3. Added the space.

4. I also tidied up an indenting issue at the bottom of the file pointed out by CodeSniffer.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
@@ -17,6 +21,36 @@
+    $this->languageManager = $language_manager;

languageManager declaration is missing.

Fixed.

Straight reroll.

1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -7,9 +7,15 @@
    namespace Drupal\custom_block;
    
   +
   

   Unneened space

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.php
   @@ -41,8 +41,8 @@ public function getConfirmText() {
   +  public function buildForm(array $form, array &$form_state) {
   +    $form = parent::buildForm($form, $form_state);
   

   The parent::buildForm needs to be at the end of the method.

New patch attached.

Moved buildForm() to bottom of method. Interdiff is with #22

Let's try that again.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
@@ -17,6 +22,43 @@
 class CustomBlockFormController extends ContentEntityFormController {
...
+   *   The action storage controller.

Hey, I thought we talk about custom blocks :)

Thanks.

Interdiff looks fine!

1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -9,7 +9,12 @@
   +use Symfony\Component\HttpFoundation\Request;
   

   Not used

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -225,11 +268,11 @@ public function delete(array $form, array &$form_state) {
   +        form_set_error('info', $this->t('A block with description %name already exists.', array(
   

   form_set_error is deprecated - should use the form builder service.

3. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.php
   @@ -41,8 +41,7 @@ public function getConfirmText() {
   -  public function buildForm(array $form, array &$form_state, Request $request = NULL) {
   -    $form = parent::buildForm($form, $form_state, $request);
   +  public function buildForm(array $form, array &$form_state) {
   

   Now you can remove the use Symfony\Component\HttpFoundation\Request; from the top of the CustomBlockDeleteForm.php file

4. The are several usages of user_access() which is also deprecated and can be replaced with $this->currentUser()->hasPermission()

Thanks, feedback done.

@benjy chatted with @tim.plunkett on IRC about the formbuilder injection and we agreed that this was a bit much - we will probably add a formSetError method to formbase but until then we should just continue to use form_set_error() - so lets revert that from this patch - sorry!

@alex, new patch attached with interdiff between #31. I did wonder if it made sense for formBase to have the formBuilder since it seems like it would be a common requirement across most forms.

31: customblockformcontroller-2060865-31.patch queued for re-testing.

New patch attached. All the tests started breaking after #2019055: Switch from field-level language fallback to entity-level language fallback was committed. Thanks to @alexpott for helping me fix that.

1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -62,23 +107,25 @@ public function form(array $form, array &$form_state) {
   +      '#description' => $this->t('A brief description of your block. Used on the <a href="@overview">Blocks administration page</a>.', array('@overview' => url('admin/structure/block'))),
   

   Can we use $this->url() here?

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
   @@ -225,11 +272,11 @@ public function delete(array $form, array &$form_state) {
          // @todo Inject this once https://drupal.org/node/2060865 is in.
   

   This @todo is for this issue.

Good catches.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
@@ -111,7 +111,7 @@ public function form(array $form, array &$form_state) {
+      '#description' => $this->t('A brief description of your block. Used on the <a href="@overview">Blocks administration page</a>.', array('@overview' => $this->url('admin/structure/block'))),

FormBase::url() takes a route name, if this doesn't fail its a bad thing. Should be $this->url('block.admin_display')

Thanks Tim.

@larowlan, I did that in #34 and then Alex and Tim suggested against it in #36 because the formBuilder would probably be injected in formBase in the near future.

Re-rolled.

Looks like how we get the entity manager changed.

54: modernize-custom-block-2060865-54.patch queued for re-testing.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
@@ -234,12 +283,11 @@ public function delete(array $form, array &$form_state) {
+        form_set_error('info', $form_state, $this->t('A block with description %name already exists.', array(

form_set_error can now be replaced by \Drupal::formBuilder()->setErrorByName($name, $form_state, $message);

@dawehner formBuilder has come up a few times in this thread. We took #36 as consensus?

Once #2145007: Convert form_set_error() in FormBase classes to use FormErrorInterface lands we won't want to inject it anyway, let's not do this here. Or get that in first, and then use $this->setFormError()

Back to RTBC or postponed then?

Personally I'd rather get this in and have a follow-up to fix all form_set_errors in block.module once #2145007: Convert form_set_error() in FormBase classes to use FormErrorInterface goes in.

Patch no longer applied because #2145007: Convert form_set_error() in FormBase classes to use FormErrorInterface changed form_set_error :)

Straight reroll.

62: custom_block-2060865-62.patch queued for re-testing.

Looks RTBC except one nitpick

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
@@ -53,23 +99,25 @@ public function form(array $form, array &$form_state) {
+    if ($this->moduleHandler->moduleExists('language')) {
+      $language_configuration = language_get_default_configuration('custom_block', $block->bundle());
...
+      // Set the correct default language.
+      if ($block->isNew()) {
+        $language_default = $this->languageManager->getLanguage($language_configuration['langcode']);

Suppose better:
if ($this..('language') && $block->isNew()) {
...
}

$language_configuration is used again further down in the form so changing that if statement would change how it works.

65: cb-modern.66.patch queued for re-testing.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
@@ -97,14 +145,14 @@ public function form(array $form, array &$form_state) {
+      '#access' => $block->isNewRevision() || $this->currentUser()->hasPermission('administer blocks'),
...
+      '#access' => $this->currentUser()->hasPermission('administer blocks'),

$account is set to $this->currentUser at the top of the form function so these change are unnecessary. Unless we remove the creation of the $account variable.

Fixed.

Committed and pushed to 8.x. Thanks!