Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Active » Needs review
FileSize
6.59 KB

_entity_form is awesome!
less code++

Status: Needs review » Needs work

The last submitted patch, block-entity-form-1983682.1.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.68 KB
8.44 KB

Swooping in :)

Status: Needs review » Needs work

The last submitted patch, custom-block-1983682-3.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.58 KB
12.51 KB

Status: Needs review » Needs work

The last submitted patch, custom-block-1983682-5.patch, failed testing.

tim.plunkett’s picture

dawehner’s picture

+++ b/core/includes/menu.incundefined
@@ -771,8 +771,15 @@ function _menu_translate(&$router_item, $map, $to_arg = FALSE) {
+    // Attempt to match this path to provide a fully built request to the
+    // acccess checker.
+    try {
+      $request->attributes->add(Drupal::service('router')->matchRequest($request));
+      $router_item['access'] = Drupal::service('access_manager')->check($route, $request);
+    }
+    catch (NotFoundHttpException $e) {
+      $router_item['access'] = FALSE;

I guess this should be moved into a separate issue.

+++ b/core/modules/block/custom_block/custom_block.routing.ymlundefined
@@ -1,6 +1,28 @@
+    custom_block: \d+

Oh does this regex support come from symfony directly, neat.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
11.17 KB

Status: Needs review » Needs work

The last submitted patch, custom-block-1983682-9.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
11.4 KB

Missed a spot.

Status: Needs review » Needs work

The last submitted patch, custom-block-1983682-11.patch, failed testing.

h3rj4n’s picture

re-rolled the patch. Tested locally but failed on the 'Custom Block translation UI' test. A verbose message gave me this:
InvalidArgumentException: Missing 'form: default' for entity 'custom_block' in Drupal\Core\Entity\EntityManager->getControllerClass() (line 166 of /var/www/html/drupal/core/lib/Drupal/Core/Entity/EntityManager.php).

It still has to do something with routing.

tim.plunkett’s picture

+++ b/core/modules/block/custom_block/custom_block.routing.ymlundefined
+++ b/core/modules/block/custom_block/custom_block.routing.ymlundefined
+     custom_block: \d+
+ ¶
+ custom_block_type_add:
...
+     _permission: 'administer blocks'
+ ¶
+ custom_block_type_edit:

Trailing whitespace

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlockType.phpundefined
+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlockType.phpundefined
-  *       "default" = "Drupal\custom_block\CustomBlockTypeFormController",

You can't get rid of "default" anymore until #2006348: Remove default/fallback entity form operation goes in.

h3rj4n’s picture

Added the 'default' in the file. Didn't found the white pace error.

The test still fails. Dunno why. Tried to add a custom block manually in a different language and I got the following error:
block_error.png

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, custom-block-1983682-15.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.26 KB
11.5 KB

Wrong form controller.
Also leftover code in custom_block.admin.inc.

Status: Needs review » Needs work
Issue tags: -WSCCI, -FormInterface, -WSCCI-conversion, -MENU_LOCAL_ACTION

The last submitted patch, custom_block-1983682-18.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +FormInterface, +WSCCI-conversion, +MENU_LOCAL_ACTION

#18: custom_block-1983682-18.patch queued for re-testing.

larowlan’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeFormController.phpundefined
@@ -21,6 +21,11 @@ public function form(array $form, array &$form_state) {
+      drupal_set_title(t('@label', array('@label' => $block_type->label())));

I think this will double encode (ie two calls to check_plain, one from drupal_set_title, one from using @)
Other than that looks RTBC

tim.plunkett’s picture

Ah that was a hack anyway, we fixed title callback for routes.

tim.plunkett’s picture

FileSize
1.44 KB

Wrong interdiff

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Unless bot says no

catch’s picture

#22: custom-block-1983682-22.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/block/custom_block/custom_block.routing.ymlundefined
@@ -24,4 +24,26 @@ custom_block_type_delete:
+custom_block_edit:
+  pattern: '/block/{custom_block}'
+  defaults:
+    _entity_form: 'custom_block.edit'
+  requirements:
+    _entity_access: 'custom_block.update'
+    custom_block: \d+

Are we sure that we want to use this route for editing a block? What about sub-requests / ESI etc...

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Is that in scope?

  $items['block/%custom_block'] = array(
    'title' => 'Edit',
    'page callback' => 'custom_block_edit',
    'page arguments' => array(1),
    'access callback' => 'entity_page_access',
    'access arguments' => array(1, 'update'),
    'file' => 'custom_block.pages.inc',
  );

is in HEAD, this is just a conversion.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

You're correct - it's out of scope :)

Committed c2e0cda and pushed to 8.x. Thanks!

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