Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Active » Postponed
larowlan’s picture

Status: Postponed » Active
larowlan’s picture

Status: Active » Needs review
FileSize
6.44 KB
ParisLiakos’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -84,13 +73,35 @@ public function add() {
+    drupal_set_title(t('Add %type custom block', array(

maybe add a todo over this to #1981644: Figure out how to deal with 'title/title callback' ?
Besides this looks good

alexpott’s picture

Now that #1913618: Convert EntityFormControllerInterface to extend FormInterface has gone in shouldn't we be using _entity_form in the route definition for this?

ParisLiakos’s picture

_entity_form: custom_block.add ? i am not sure what happens here, cause we need the have the dummy new entity with the type assigned for this to work..but i would lie, havent tried that yet

larowlan’s picture

As discussed in irc, we can't use _entity_form in its current form because we have a dynamic type. So even if we could change it to custom_block.default.basic to nominate the bundle, we'd be hard-coding the bundle when we need to take that from the slug.

Fixes request at #4

larowlan’s picture

Damn it, wrong patch

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

great, thanks! i think its ready to go then, if alex does not object..but Crell also agreed in WSSCI's meeting today that we should better use drupal_set_title there *temporarily* instead of another callbacks

andypost’s picture

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Thanks guys! Regarding #7:

we can't use _entity_form in its current form because we have a dynamic type

I think we should instead fix _entity_form to allow for dynamic types, because we want to make sure to get these route definitions correct the first time. If this pattern gets introduced here, it's going to be copy/pasted into other similar forms, so let's set a good example.

After that, we can change this hunk to use _entity_form instead of _content:

+++ b/core/modules/block/custom_block/custom_block.routing.ymlundefined
@@ -4,3 +4,9 @@ custom_block_add_page:
+  defaults:
+    _content: 'Drupal\custom_block\Controller\CustomBlockController::addForm'
larowlan’s picture

Status: Needs work » Needs review
FileSize
11.81 KB
7.56 KB

So this adds a _values option to the defaults in the yml that hands _entity_form a callable for initializing the entity object's default values.
Works for me™

larowlan’s picture

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.phpundefined
@@ -15,16 +16,41 @@
+    // request attributes, skipping $form and $form_state.

don't need refs to $form and $form_state here c/p fail

larowlan’s picture

whoops, yes it does -review fail

larowlan’s picture

So this removes the need for reflection.
The _values function gets passed the request instead - and can get the attributes it needs.

larowlan’s picture

And this one gets the _values from $request->attributes which is already in scope, removing the need to duplicate much of HtmlFormController::content.

tim.plunkett’s picture

This looks pretty awesome.

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.phpundefined
@@ -8,6 +8,7 @@
+use Symfony\Component\HttpFoundation\Response;

Where is Response used? or is this left over from an earlier patch.

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.phpundefined
@@ -15,13 +16,14 @@
 class HtmlEntityFormController extends HtmlFormController {
 
+
   /**

No extra line needed

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.phpundefined
@@ -56,7 +58,26 @@ protected function getFormObject(Request $request, $form_arg) {
+      // A default values callback has been provided.
+      if (($_values = $request->attributes->get('_values')) &&
+        // Callable is in format class::method.
+          strpos($_values, '::') !== FALSE) {

I think these comments should be combined, and the if() should be on one line

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -9,7 +9,7 @@
+use Drupal\custom_block\Plugin\Core\Entity\CustomBlockType;

@@ -84,13 +73,49 @@ public function add() {
+   * @param \Drupal\custom_block\Plugin\Core\Entity\CustomBlockType $custom_block_type
...
+  protected function addForm(CustomBlockType $custom_block_type) {

There is now a \Drupal\custom_block\CustomBlockTypeInterface that should be used to typehint instead

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -84,13 +73,49 @@ public function add() {
+      return $this->addForm($type);
...
+   * Presents the custom block creation form.

Maybe its just not clear from the patch context, but why is this a separate method?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -84,13 +73,49 @@ public function add() {
+    return entity_get_form($block);

This will seem much less hacky when #1982980: Move entity_get_form to Drupal\Core\Entity\EntityManager::getForm() goes in

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.phpundefined
@@ -42,6 +42,14 @@ protected function prepareEntity() {
+    if ($block->isNew()) {

This should eventually be a check for $this->operation, I switched all the calls around in another issue

larowlan’s picture

Fixes issues identified at #17

larowlan’s picture

meh wrong use statement

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.phpundefined
@@ -56,7 +56,25 @@ protected function getFormObject(Request $request, $form_arg) {
-      $entity = $manager->getStorageController($entity_type)->create(array());
+      $values = array();
+      // A default values callback has been provided and the callable is in
+      // class::method format.
+      if (($_values = $request->attributes->get('_values')) && strpos($_values, '::') !== FALSE) {
+        // Extract the class and method from the given string.
+        list($class, $method) = explode('::', $_values);
+        // The default values callback is in a ControllerInterface object.
+        if (in_array('Drupal\Core\ControllerInterface', class_implements($class))) {
+          $defaults_class = $class::create($this->container);
+        }
+        else {
+          $defaults_class = new $class();
+        }
+
+        // Use _values callable to build default values for the entity.
+        $values = call_user_func(array($defaults_class, $method), $request);
+      }
+
+      $entity = $manager->getStorageController($entity_type)->create($values);

This looks about right. Matches much of our other controller handlers.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -9,7 +9,7 @@
+use Drupal\custom_block\Plugin\Core\Entity\CustomBlockTypeInterface;

This looks wrong. Shouldn't it be Drupal\custom_block\CustomBlockTypeInterface?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -84,13 +73,37 @@ public function add() {
+      $block = $this->entityManager->getStorageController('custom_block')->create(array(
+        'type' => $custom_block_type->id()
+      ));

I don't get this bit. Should this have been handled elsewhere?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.phpundefined
@@ -42,6 +42,14 @@ protected function prepareEntity() {
+    if ($block->isNew()) {

This should ideally be checking $this->operation == 'add', but we don't have that yet.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

My review was for the wrong patch.

The last two bits are actually cleaned up in #1983682: Convert applicable custom_block_menu() entries to use _entity_form in routing.yml, so this on its own is great.

@larowlan, can we get an improved issue title to imply the new scope of the issue?

larowlan’s picture

Title: Convert block/add/{%custom_block_type} to Controller » Convert block/add/{%custom_block_type} to Controller, add _values to allow default values for _entity_form
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Entity/HtmlEntityFormController.phpundefined
@@ -56,7 +56,25 @@ protected function getFormObject(Request $request, $form_arg) {
+      if (($_values = $request->attributes->get('_values')) && strpos($_values, '::') !== FALSE) {
+        // Extract the class and method from the given string.
+        list($class, $method) = explode('::', $_values);
+        // The default values callback is in a ControllerInterface object.
+        if (in_array('Drupal\Core\ControllerInterface', class_implements($class))) {
+          $defaults_class = $class::create($this->container);
+        }
+        else {
+          $defaults_class = new $class();
+        }
+
+        // Use _values callable to build default values for the entity.
+        $values = call_user_func(array($defaults_class, $method), $request);
+      }
+
+      $entity = $manager->getStorageController($entity_type)->create($values);

We need to through an exception here is the request has a _values and it does not contain a ::

Crell’s picture

larowlan, Tim Plunkett, Alex Pott, and I discussed this at the Extended Sprints. The general consensus was "we don't have enough time before freeze to figure out how to do dynamic default values well, so it's better to not do them than to do them badly."

The _values implementation above feels icky, and is duplicating code from elsewhere (ie, ControllerResolver), which suggests we'd want to us that soon enough, which ends up just getting weird. So let's just not do that. We'll go back to having a trivial wrapper method, which is no worse than the page callbacks that were just trivial wrappers around drupal_get_form() rather than using drupal_get_form() as a page callback. There's no DX regression in doing so.

So, back to the original plan. larowlan, can you provide an updated patch for testbot?

larowlan’s picture

Title: Convert block/add/{%custom_block_type} to Controller, add _values to allow default values for _entity_form » Convert block/add/{%custom_block_type} to Controller
Status: Needs work » Needs review
FileSize
6.51 KB

back to old title.

would you believe the patch at 8 still applies, wonder what bot says

smiletrl’s picture

Issue tags: +Need tests
FileSize
614 bytes
6.58 KB

Missing converters in routing file:) Also need tests.

andypost’s picture

+++ b/core/modules/block/custom_block/custom_block.routing.ymlundefined
@@ -14,6 +14,9 @@ custom_block_add_page:
+    converters:
+      custom_block_type: 'custom_block_type'

there's no conversion!

andypost’s picture

Status: Needs review » Needs work
Issue tags: -Need tests +CodeSprintUA

There's tests for that

 \Drupal\custom_block\Tests\CustomBlockCreationTest
 \Drupal\custom_block\Tests\CustomBlockTypeTest

tested manually #25 and it works

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -84,13 +73,36 @@ public function add() {
+   * @param Drupal\custom_block\Plugin\Core\Entity\CustomBlockType $custom_block_type
+   *   The custom block type to add.

needs \

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -84,13 +73,36 @@ public function add() {
+    $options = array();

not used

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.phpundefined
@@ -84,13 +73,36 @@ public function add() {
+    if (($theme = $this->request->attributes->get('theme')) && in_array($theme, array_keys(list_themes()))) {

do we really need theme here?

smiletrl’s picture

My fault -- I didn't clear cache after I applied patch at #25.
And apparently, test has covered.

    $this->drupalGet('block/add/' . $type->id());
    $this->assertResponse(200, 'The new block type can be accessed at bloack/add.');

Rerolled according to #28

smiletrl’s picture

Status: Needs work » Needs review

theme is probably needed here.

dawehner’s picture

Nice! This is RTBC once it's green.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

curl https://drupal.org/files/block-add-type-1978166.28.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  6638  100  6638    0     0   2779      0  0:00:02  0:00:02 --:--:--  3588
error: patch failed: core/modules/block/custom_block/custom_block.pages.inc:35
error: core/modules/block/custom_block/custom_block.pages.inc: patch does not apply
larowlan’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.54 KB

Re-roll

Status: Reviewed & tested by the community » Needs work

The last submitted patch, block-add-type-1978166.33.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.42 KB

That last reroll had the defaults() stuff in it, I think it was the wrong patch.

This cleans up the CustomBlockController to match the more recent conversions.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Provided bot agrees

effulgentsia’s picture

Issue tags: -Needs reroll
FileSize
8.38 KB

HEAD moves fast. I was about to remove the "Needs reroll" tag, but turned out #36 needed yet another reroll. This is a straight reroll, so leaving at RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6f25f15 and pushed to 8.x. Thanks!

xjm’s picture

Title: Convert block/add/{%custom_block_type} to Controller » [HEAD BROKEN] Convert block/add/{%custom_block_type} to Controller
Category: task » bug
Priority: Normal » Critical
Status: Fixed » Reviewed & tested by the community

The tests from this are currently breaking head:
git revert 6f25f1588523

tim.plunkett’s picture

Let's just fix the fail. It was an overly specific typehint, and the class got removed.

webchick’s picture

Title: [HEAD BROKEN] Convert block/add/{%custom_block_type} to Controller » Convert block/add/{%custom_block_type} to Controller
Category: bug » task
Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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