Files: 
CommentFileSizeAuthor
#41 custom-block-1978166-41.patch2.48 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom-block-1978166-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#38 custom-block-1978166-38.patch8.38 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 57,791 pass(es).
[ View ]
#36 custom-block-1978166-36.patch8.42 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,165 pass(es).
[ View ]
#34 block-add-type-1978166.33.patch7.54 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,883 pass(es), 57 fail(s), and 1 exception(s).
[ View ]
#29 block-add-type-1978166.28.patch6.48 KBsmiletrl
PASSED: [[SimpleTest]]: [MySQL] 56,084 pass(es).
[ View ]
#29 interdiff_25-28.txt1.26 KBsmiletrl
#26 block-add-type-1978166.26.patch6.58 KBsmiletrl
PASSED: [[SimpleTest]]: [MySQL] 56,096 pass(es).
[ View ]
#26 interdiff_25-26.txt614 bytessmiletrl
#25 block-add-type-1978166.25.patch6.51 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 56,915 pass(es).
[ View ]
#19 block-add-type-1978166.19.interdiff.txt757 byteslarowlan
#19 block-add-type-1978166.19.patch9.41 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,152 pass(es).
[ View ]
#18 block-add-type-1978166.18.interdiff.txt4.09 KBlarowlan
#18 block-add-type-1978166.18.patch9.42 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#16 block-add-type-1978166.16.interdiff.txt3.38 KBlarowlan
#16 block-add-type-1978166.16.patch9.94 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,546 pass(es).
[ View ]
#15 block-add-type-1978166.15.interdiff.txt2.6 KBlarowlan
#15 block-add-type-1978166.15.patch11.36 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,502 pass(es).
[ View ]
#12 block-add-type-1978166.12.interdiff.txt7.56 KBlarowlan
#12 block-add-type-1978166.12.patch11.81 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,846 pass(es).
[ View ]
#8 block-add-type-1978166.8.interdiff.txt949 byteslarowlan
#8 block-add-type-1978166.8.patch6.5 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,377 pass(es).
[ View ]
#7 block-add-type-1978166.7.interdiff.txt0 byteslarowlan
#7 block-add-type-1978166.7.patch8.57 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#3 block-add-type-1978166.1.patch6.44 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,371 pass(es).
[ View ]

Comments

Status:Active» Postponed

Status:Postponed» Active

Status:Active» Needs review
StatusFileSize
new6.44 KB
PASSED: [[SimpleTest]]: [MySQL] 55,371 pass(es).
[ View ]

+++ 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

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

_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

StatusFileSize
new8.57 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new0 bytes

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

StatusFileSize
new6.5 KB
PASSED: [[SimpleTest]]: [MySQL] 55,377 pass(es).
[ View ]
new949 bytes

Damn it, wrong patch

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

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'

Status:Needs work» Needs review
StatusFileSize
new11.81 KB
PASSED: [[SimpleTest]]: [MySQL] 55,846 pass(es).
[ View ]
new7.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™

+++ 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

whoops, yes it does -review fail

StatusFileSize
new11.36 KB
PASSED: [[SimpleTest]]: [MySQL] 55,502 pass(es).
[ View ]
new2.6 KB

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

StatusFileSize
new9.94 KB
PASSED: [[SimpleTest]]: [MySQL] 55,546 pass(es).
[ View ]
new3.38 KB

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

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

StatusFileSize
new9.42 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new4.09 KB

Fixes issues identified at #17

StatusFileSize
new9.41 KB
PASSED: [[SimpleTest]]: [MySQL] 57,152 pass(es).
[ View ]
new757 bytes

meh wrong use statement

+++ 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.

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?

Title:Convert block/add/{%custom_block_type} to ControllerConvert block/add/{%custom_block_type} to Controller, add _values to allow default values for _entity_form

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 ::

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?

Title:Convert block/add/{%custom_block_type} to Controller, add _values to allow default values for _entity_formConvert block/add/{%custom_block_type} to Controller
Status:Needs work» Needs review
StatusFileSize
new6.51 KB
PASSED: [[SimpleTest]]: [MySQL] 56,915 pass(es).
[ View ]

back to old title.

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

Issue tags:+Need tests
StatusFileSize
new614 bytes
new6.58 KB
PASSED: [[SimpleTest]]: [MySQL] 56,096 pass(es).
[ View ]

Missing converters in routing file:) Also need tests.

+++ 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!

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?

StatusFileSize
new1.26 KB
new6.48 KB
PASSED: [[SimpleTest]]: [MySQL] 56,084 pass(es).
[ View ]

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

Status:Needs work» Needs review

theme is probably needed here.

Nice! This is RTBC once it's green.

Status:Needs review» Reviewed & tested by the community

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new7.54 KB
FAILED: [[SimpleTest]]: [MySQL] 55,883 pass(es), 57 fail(s), and 1 exception(s).
[ View ]

Re-roll

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new8.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,165 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Provided bot agrees

Issue tags:-Needs reroll
StatusFileSize
new8.38 KB
PASSED: [[SimpleTest]]: [MySQL] 57,791 pass(es).
[ View ]

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.

Status:Reviewed & tested by the community» Fixed

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

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

StatusFileSize
new2.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom-block-1978166-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Title:[HEAD BROKEN] Convert block/add/{%custom_block_type} to ControllerConvert 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.