Posted by xjm

This issue is a followup for #1781372: Add an API for listing (configuration) entities and #1783964: Allow entity types to provide menu items.

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

Current behavior

list-controller-no-add-link.jpg

Intended behavior

list-controller-add-link.jpg

CommentFileSizeAuthor
#120 rerolldiff_110-120.txt43.4 KBayush.khare
#120 1798540-120.patch61.75 KBayush.khare
#110 1798540-110.patch61.45 KBjofitz
#106 interdiff-1798540-104-106.txt17 KBmartin107
#106 1798540-106.patch63.81 KBmartin107
#104 interdiff-1798540-102-104.txt50.01 KBmartin107
#104 1798540-104.patch63.74 KBmartin107
#102 interdiff-1798540-100-102.txt1.76 KBmartin107
#102 1798540-102.patch54.45 KBmartin107
#100 interdiff-1798540-98-100.txt1.83 KBmartin107
#100 1798540-100.patch54.49 KBmartin107
#98 interdiff-1798540-96-98.txt3.49 KBmartin107
#98 1798540-98.patch52.65 KBmartin107
#96 interdiff-1798540-94-96.txt786 bytesmartin107
#96 1798540-96.patch50.02 KBmartin107
#94 interdiff-1798540-92-94.txt681 bytesmartin107
#94 1798540-94.patch50.02 KBmartin107
#92 interdiff-1798540-90-92.txt1.67 KBmartin107
#92 1798540-92.patch50.01 KBmartin107
#90 interdiff.txt1.41 KBpfrenssen
#90 1798540-90.patch48.34 KBpfrenssen
#88 1798540-88.patch47.6 KBpfrenssen
#86 interdiff.patch2.39 KBpfrenssen
#86 1798540-86.patch46.46 KBpfrenssen
#84 1798540-84.patch46.49 KBpfrenssen
#83 1798540-83.patch46.44 KBpfrenssen
#80 interdiff.txt1.39 KBpfrenssen
#80 1798540-80.patch46.41 KBpfrenssen
#78 1798540-78.patch45.03 KBpfrenssen
#78 interdiff.txt3.51 KBpfrenssen
#76 interdiff.txt35.04 KBpfrenssen
#76 1798540-76.patch41.81 KBpfrenssen
#73 interdiff.txt2.39 KBpfrenssen
#73 1798540-73.patch6.78 KBpfrenssen
#71 1798540-71.patch4.38 KBpfrenssen
#71 interdiff.txt3.17 KBpfrenssen
#67 interdiff-64-67.txt555 bytespk188
#67 1798540-67.patch2.74 KBpk188
#64 1798540-64.patch2.74 KBpfrenssen
#49 drupal_1798540_48.patch14.91 KBXano
#46 drupal_1798540_46.patch14.94 KBXano
#40 drupal_1798540_40.patch40.73 KBjamesquinton
#36 drupal_1798540_36.patch28.86 KBXano
#36 interdiff.txt16.55 KBXano
#34 drupal_1798540_34.patch16.64 KBXano
#31 drupal_1798540_31.patch14.06 KBXano
#31 interdiff.txt13.25 KBXano
#30 drupal-new-entity-link-1798540-29.patch7.68 KBjamesquinton
#21 drupal-new-entity-link-1798540-20.patch7.44 KBjamesquinton
#19 drupal-new-entity-link-1798540-19.patch7.42 KBjamesquinton
#17 drupal-new-entity-link-1798540-17.patch7.58 KBjamesquinton
#14 drupal-new-entity-link-1798540-13.patch3.07 KBjamesquinton
#14 drupal-new-entity-link-1798540-13.patch3.07 KBjamesquinton
#13 drupal-new-entity-link-1798540-13.patch5.09 MBjamesquinton
#4 drupal-1798540-4.patch3.39 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue tags: +Usability
xjm’s picture

Issue tags: +Entity system, +VDC, +Configurables
xjm’s picture

Title: Add link to add a new entity inside the entity list controller table when no items are listed » Add link to add a new entity in an empty entity list controller table
xjm’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3.39 KB

I'm wondering whether this should be part of the interface, so i started without it.

It feels ugly to remove all existing config entities in the test to be able to check this
functionality.

sun’s picture

Hrm. We really need an entity operations/actions API. That has become crystal clear through a dozen of issues already.

I rather object to working around that missing puzzle piece even further. Let's fix the root cause/limitation instead.

An entity operations/actions API would inherently depend on #1696660: Add an entity access API for single entity access, since that introduces generic access handling, so each operation/action can be properly checked for access before it is exposed.

andypost’s picture

Closely related #1839516: Introduce entity operation providers

The "Add item/sub-item" could be a operations

sun’s picture

Issue tags: +Blocks-Layouts

The proposed change here attempts to hardcode a 1:1 relationship between an entity and where it appears in a listing, or perhaps rather, where its "Add form" is located.

However, architecturally, that's really not where we want to go. In case you know http://drupal.org/project/context_admin, that's much more in line with our architectural goals — entities can be listed in various contexts, and within each context, you are able to add a new, but also manage existing.

In a bit more abstract RESTful speak, this means:

List: GET /entity
List: GET /admin/content/entity

Add:  POST /entity
Add:  POST /admin/content/entity

Edit: POST /entity/{id}
Edit: POST /admin/content/entity/{id}
...

Unfortunately, the pure RESTful pattern cannot be applied to web-based form interaction. A typical way to work around it would be to replace the {id} with a static placeholder string that would be invalid as a regular/true ID otherwise; e.g.:

Add:  GET|POST /entity/:new:
Add:  GET|POST /admin/content/entity/:new:

In turn, regardless of the current context, you're able to retrieve the URI for creating/adding a new entity based on its regular URI pattern:

$entity = entity_create('foo', array(
  'id' => ':new:',
);
$uri = $entity->uri();

I'm aware that we didn't make much progress on contexts yet, but the proposed change here with a hard 1:1 binding would implicitly move us into a direction that is not aware of contexts, whereas we want to go into the opposite direction and make this stuff context-aware.

Tagging with Blocks/Layouts, in the hope that @EclipseGc and others might be able to chime in.

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Usability, +Needs tests, +Entity system, +VDC, +Blocks-Layouts, +Configurables

The last submitted patch, drupal-1798540-4.patch, failed testing.

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -64,6 +64,23 @@ public function load() {
+    if (isset($this->entityInfo['uri_prefix']) && isset($this->entityInfo['label'])) {

I think all entities should have uri() method implemented or use parent

dawehner’s picture

Well, but how can we use uri() to create a link to a creation for of entities? I don't see how this can workout.

dawehner’s picture

Issue summary: View changes

Typo.

damiankloip’s picture

Yeah, uri() won't really help us as we don't have an entity. Also I thought #1970360: Entities should define URI templates and standard links may have helped us here, but it seems that the summary mentions create urls but it didn't make it into the patch?

damiankloip’s picture

Issue summary: View changes

Removing myself from the author field so I can unfollow. --xjm

jamesquinton’s picture

Assigned: Unassigned » jamesquinton
Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.09 MB

Re-rolled patch and updated.

jamesquinton’s picture

Woops..! Here's the correct patch.

The last submitted patch, 13: drupal-new-entity-link-1798540-13.patch, failed testing.

The last submitted patch, 14: drupal-new-entity-link-1798540-13.patch, failed testing.

jamesquinton’s picture

Updated and added link to the following lists:

  • Date formats
  • Custom block types
  • Custom blocks
  • Node
  • Image styles
  • Responsive image mappings

I don't think every entity list needs this, for example User Roles - where you can’t remove the default roles, and therefore never have an empty list.

“entity-add-form” link key could probably be named better.

jamesquinton’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Doh - rerolling without my .gitignore in the patch...

jamesquinton’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.42 KB

Rerolled

dawehner’s picture

Thank you!

  1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -202,6 +203,24 @@ public function render() {
    +   *
    +   * @return string Add link
    

    A few more words would be cool

  2. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -202,6 +203,24 @@ public function render() {
    +      $link_object = new Url($link_template);
    ...
    +        $link_object
    

    Let's call it just url or url_object.

jamesquinton’s picture

Cool - agreed!

Here's a new patch with your suggestions implemented...

damiankloip’s picture

+++ b/core/modules/node/lib/Drupal/node/Entity/NodeType.php
@@ -37,7 +37,8 @@
  *     "add-form" = "node.add",
  *     "edit-form" = "node.type_edit",
...
+ *     "delete-form" = "node.type_delete_confirm",
+ *     "add-entity-form" = "node.type_add"

add-entity-form seems to break the pattern of $action-form.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

At NYCCamp we had a long discussion about the naming, and the weirdness of NodeType having a route for node.add and node.type_add, and what to name them.

If we really care about the naming, we should open a dedicated issue, because I'm pretty sure the existing "add-form" = "node.add", pattern is wrong.

Awesome work @jamesquinton, (hopefully) congrats on your first core patch!

damiankloip’s picture

IMO having the actual node type as add-entity-form is even more confusing. So add-form does is not for the actual entity type of the annotation it's on but for nodes, whereas add-entity-form is for the current entity. Maybe at least add-type-form?

Looks like this was happening before this, but why are node and node type mixing the link templates so much? This just seems wrong.

tstoeckler’s picture

So the reason why "node.add_form" (i.e. node/add/{type}) is on NodeType instead of Node is because it has the node type as a route parameter and not a node. I also think that is fairly weird but I also have no clue how to fix this.

damiankloip’s picture

:/

jamesquinton’s picture

Assigned: jamesquinton » Unassigned

Unasigning myself as I think this is now more of a naming issue now...

I do agree that it seems strange for node.add_form to be on NodeType instead of Node.

Interestingly, custom blocks are kind of similar - but with Custom Block Types, there is "custom_block.type_add".

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: drupal-new-entity-link-1798540-20.patch, failed testing.

jamesquinton’s picture

Assigned: Unassigned » jamesquinton
Issue tags: +Needs reroll
jamesquinton’s picture

Assigned: jamesquinton » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.68 KB

Re-rolled patch.

Xano’s picture

  1. I changed the link template name from add-entity-form to add, since it's obvious that this is about entities, and the route may not point to form pages. The one for nodes, for instance, points to a page where users can select the type of which they want to add a node.
  2. I added access control to the link. In order to do that I had to change the behavior of EntityAccessControllerInterface a little.
  3. I injected all dependencies into EntityListBuilder.

The last submitted patch, 30: drupal-new-entity-link-1798540-29.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 31: drupal_1798540_31.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
16.64 KB

Status: Needs review » Needs work

The last submitted patch, 34: drupal_1798540_34.patch, failed testing.

Xano’s picture

FileSize
16.55 KB
28.86 KB

My apologies for not reading the issue thoroughly enough and explaining why I made some changes. Here's my reasoning

  1. I changed the link template, because not all entity types have a single add form, namely those with bundles. Those have pages you have to select the correct bundle at, before you get to the form. For consistency with the other templates I reverted this change to add-entity-form, though.
  2. I added a few smaller methods to make it easier for child classes to change the human-readable strings for when the table is empty. We have a sane default, but entity type names are too different from one another for this default to be correct in most cases.
  3. I would very much like a review on my EntityAccessControllerInterface change from last patch. It does not break any existing code, but it may not work properly with existing code either. It does allow us to check access to the add-entity-form link template's route for entity types that have bundles, though.

This patch reverts the link template name, and fixes some test failures.

@jamesquinton: I learned that this is your first core issue and you started on it as part of a sprint. I tried to make this patch a starting point for you to finish fixing the test failures and learn something about dependency injection. See the interdiff for how some of the previous failures were fixed. If you are no longer interested or cannot finish this issue, that is fine and I will do it. Otherwise, the honor is yours and I will review your patches.

jamesquinton’s picture

Assigned: Unassigned » jamesquinton

@Xano - thanks for the detailed explanation! Definitely still interested, I'll take a look (hopefully today)...

Xano’s picture

Status: Needs work » Needs review

Cool!

I forgot to trigger the bot. Let's see how much fails.

Status: Needs review » Needs work

The last submitted patch, 36: drupal_1798540_36.patch, failed testing.

jamesquinton’s picture

FileSize
40.73 KB

OK - I'm having trouble running tests locally at the moment, but hopefully this should reduce the number of errors...

jamesquinton’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 40: drupal_1798540_40.patch, failed testing.

jamesquinton’s picture

Issue tags: +drupalcampfi
jamesquinton’s picture

Assigned: jamesquinton » Unassigned
Issue tags: -drupalcampfi

I'm starting to think this may be better suited to someone who is more comfortable with dependency injections, as this is still a relatively new concept to me!

I think I will have to bail out of this one, at least until I am a bit more familiar with dependency injections...

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

I think this has gotten a little out of hand with the DI, I don't blame you for being confused @jamesquinton. I'm going to try and get this back on track.

Xano’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
14.94 KB

Unassigning, since Tim just left the sprint and is on a plane.

This patch removes the dependency injection.

Xano’s picture

46: drupal_1798540_46.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 46: drupal_1798540_46.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
14.91 KB

Re-roll.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -203,6 +208,44 @@ public function render() {
    +    return $this->t('Add one.');
    

    Did we thought about not using a dot at the end? Did we considered to put @label of $this->enttityType->getLabel() in there?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -203,6 +208,44 @@ public function render() {
    +    if ($this->entityType->hasLinkTemplate('add-entity-form') && \Drupal::entityManager()->getAccessController($this->entityTypeId)->createAccess()) {
    +      return \Drupal::linkGenerator()->generate(
    

    I can haz injections or at least helper methods?

  3. +++ b/core/modules/node/src/NodeListBuilder.php
    @@ -49,8 +49,7 @@ public function __construct(EntityTypeInterface $entity_type, EntityStorageInter
    +  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {    return new static(
           $entity_type,
           $container->get('entity.manager')->getStorage($entity_type->id()),
           $container->get('date')
    

    This looks wrong.

Xano’s picture

Did we thought about not using a dot at the end? Did we considered to put @label of $this->enttityType->getLabel() in there?

Yes. See the overrides in the other classes. These are sensible defaults that are grammatically and stylistically correct, as we can't dynamically do that by using entity type names.

I can haz injections or at least helper methods?

It was decided that that's for a follow-up and we focus on just adding the link here.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, all people know that you are lazy.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/node/src/NodeListBuilder.php
@@ -49,8 +49,7 @@ public function __construct(EntityTypeInterface $entity_type, EntityStorageInter
-  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
-    return new static(
+  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {    return new static(

This change is just wrong.

The last submitted patch, 49: drupal_1798540_48.patch, failed testing.

Status: Needs work » Needs review

Xano queued 49: drupal_1798540_48.patch for re-testing.

dawehner’s picture

Status: Needs review » Needs work

so.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Issue tags: -Blocks-Layouts

Not actively part of the Blocks-Layouts work.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pfrenssen’s picture

Version: 8.3.x-dev » 8.4.x-dev
Issue tags: +Needs reroll

I tried to rebase this onto 8.4.x for the third anniversary of the patch but the code base has changed dramatically in the meantime. It seems like it will be simpler to reroll it from scratch.

pfrenssen’s picture

I went through the comments on this issue and at the time there was a lot of confusion about the link template to use. The patch settled on add-entity-form but this was still being debated.

In the meantime this has been cleared up, there is an add-page link template (ref. DefaultHtmlRouteProvider::getAddPageRoute()) for entity types that have bundles, leading to a page where a bundle can be chosen for the entity to create, such as node/add. There is also an add-form route (ref. DefaultHtmlRouteProvider::getAddFormRoute()) which leads directly to the create form. It requires a bundle parameter if the entity type has bundles.

It does seem that there is no wide support for add-page and add-form yet but this is trivial to add. This can be done in this issue as was done in the original patch.

In the entity list builder we can check if the entity has bundles and point to add-page if it is bundlified, and add-form if it isn't.

I also reviewed the patch and from a DX standpoint I think it's great to add a getEmptyText() method. This allows developers to easily control the exact text that is shown for their entities. It's not so great though that the final composition of the empty text is still handled in the render() function, so ultimately the developer _doesn't_ have control over the text. Let's do the concatenation of the two strings inside getEmptyText().

joachim’s picture

I think #62 sounds like a good plan except for this point:

> In the entity list builder we can check if the entity has bundles and point to add-page if it is bundlified, and add-form if it isn't.

An entity type might not be using DefaultHtmlRouteProvider, and might not be following the conventions for link templates that are used there. So rather than check for the entity type using bundles, I think we should check the link templates. In pseudocode, the logic would be:

if (has add-page) {
  use add-page
}
elseif (has add-form) {
  use add-form
}
else {
  don't show an add link as we don't know what to do.
}
pfrenssen’s picture

FileSize
2.74 KB

@joachim, thanks, that sounds like the best approach!

Started the work on the reroll. No interdiff since this is starting again from scratch, the source tree has diverged too much to make a rebase feasible. This is not yet finished, so not burdening the bot with it.

pk188’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 64: 1798540-64.patch, failed testing. View results

pk188’s picture

Status: Needs work » Needs review
FileSize
2.74 KB
555 bytes

Did some changes. Please check once.

Status: Needs review » Needs work

The last submitted patch, 67: 1798540-67.patch, failed testing. View results

jofitz’s picture

@pk188 Be aware that this is a work in process containing psuedo-code, hence why @pfrenssen said in #64:

This is not yet finished, so not burdening the bot with it.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pfrenssen’s picture

Rerolled to 8.5.x, converted the pseudocode into PHP, and addressed the @todo's in the code. This is completely untested yet.

pfrenssen’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Just setting to needs review to get test feedback from the bot. This still needs work.

pfrenssen’s picture

FileSize
6.78 KB
2.39 KB

Added some of the missing link templates. It's a lot of work actually to check all existing entity types and hunt for their forms.

The last submitted patch, 71: 1798540-71.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 73: 1798540-73.patch, failed testing. View results

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
41.81 KB
35.04 KB

Addressing test failures.

Status: Needs review » Needs work

The last submitted patch, 76: 1798540-76.patch, failed testing. View results

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
3.51 KB
45.03 KB

Fixing a few more failures.

Some of the new link templates have legacy route names that predate the structured entity paths and are not discovered automatically. It appears that the Symfony router does not support aliases for routes so I am overriding the EntityInterface::toUrl() method instead.

Status: Needs review » Needs work

The last submitted patch, 78: 1798540-78.patch, failed testing. View results

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
46.41 KB
1.39 KB

Fixing another test. Still need to figure out where the REST tests get their expected data from.

Status: Needs review » Needs work

The last submitted patch, 80: 1798540-80.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pfrenssen’s picture

pfrenssen’s picture

FileSize
46.49 KB

Another reroll.

joachim’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -264,4 +273,63 @@ protected function ensureDestination(Url $url) {
    +   *   The HTML link or FALSE if no link is available.
    

    Is there any benefit to returning FALSE instead of NULL? NULL is easier to read, as the return statements are simpler.

    You can then also use isset() instead of !empty() for the condition that checks the return, which is easier to read as it's not a double negative.

  2. +++ b/core/modules/aggregator/src/Entity/Feed.php
    @@ -31,6 +31,7 @@
    + *     "add-form": "/aggregator/sources/add",
    

    Typo. Should be = not :

  3. +++ b/core/modules/node/src/Entity/Node.php
    @@ -70,6 +70,8 @@
    + *     "add-page": "/node/add",
    + *     "add-form": "/node/add/{node_type}",
    

    Same here.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
46.46 KB
2.39 KB

Thanks for the review, Joachim! Fixed the remarks.

The last submitted patch, 86: 1798540-86.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

pfrenssen’s picture

FileSize
47.6 KB

Straight reroll.

Status: Needs review » Needs work

The last submitted patch, 88: 1798540-88.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
48.34 KB
1.41 KB

Fixed failures regarding the BlockContent entity.

Status: Needs review » Needs work

The last submitted patch, 90: 1798540-90.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Needs review
FileSize
50.01 KB
1.67 KB

Another common point of failure is RoleListBuilder

Status: Needs review » Needs work

The last submitted patch, 92: 1798540-92.patch, failed testing. View results

martin107’s picture

Status: Needs work » Needs review
FileSize
50.02 KB
681 bytes

more commas,

Status: Needs review » Needs work

The last submitted patch, 94: 1798540-94.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Needs review
FileSize
50.02 KB
786 bytes

Less errors.

Status: Needs review » Needs work

The last submitted patch, 96: 1798540-96.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Needs review
FileSize
52.65 KB
3.49 KB

A) fixed up MediaListBuilder::construct()

B) As this patch adds new test code it should use Object:class strings as the error reporting will be sharper, more semantically meaningfull when changes elsewhere introduce failures.

Status: Needs review » Needs work

The last submitted patch, 98: 1798540-98.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Needs review
FileSize
54.49 KB
1.83 KB

WorkspaceListBuilder.

Status: Needs review » Needs work

The last submitted patch, 100: 1798540-100.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Needs review
FileSize
54.45 KB
1.76 KB

Just sweeping up some minor coding standard errors...

We don't need to introduce a stray use of

Drupal\Component\Utility\Unicode;

Status: Needs review » Needs work

The last submitted patch, 102: 1798540-102.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Assigned: Unassigned » martin107
Status: Needs work » Needs review
FileSize
63.74 KB
50.01 KB

I want rid of a small inconsistency

code we are introducing in EntityListBuilder:getAddLink() introduces a hidden dependancy on the LinkGenerator.

Fixing this small conceptual change .. ripples through 23 other files.

Status: Needs review » Needs work

The last submitted patch, 104: 1798540-104.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Needs review
FileSize
63.81 KB
17 KB

In #104

I systematically added a coding standard error regarding @param ... link_generator.

and introduced a error..my bad

Status: Needs review » Needs work

The last submitted patch, 106: 1798540-106.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

martin107’s picture

Assigned: martin107 » Unassigned
Issue tags: +Needs reroll

Needs reroll.

jofitz’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
61.45 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 110: 1798540-110.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ayush.khare’s picture

Reroll #110 for 10.1.x

larowlan’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -65,11 +82,17 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +    $this->linkGenerator = $link_generator;
    

    Is the disruption of injecting the link generator worth it?

    All the other links we generate in this class (e.g. in operations) don't need/use it so that makes me feel we may not need it.

    That would dramatically reduce the complexity of this patch and the disruption to contrib.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -248,4 +269,61 @@ protected function ensureDestination(Url $url) {
    +        Url::fromUri('internal:' . $this->entityType->getLinkTemplate('add-page'))
    ...
    +        Url::fromUri('internal:' . $this->entityType->getLinkTemplate('add-form'))
    

    Is internal: our only option here?

    \Drupal\Core\Entity\Controller\EntityController::addPage uses Link::createFromRoute

  3. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
    @@ -248,4 +269,61 @@ protected function ensureDestination(Url $url) {
    +    elseif ($this->entityType->hasLinkTemplate('add-form')) {
    

    This could return an exception if there is no add page link template, but there is an add-form link template but it requires additional parameters. So we probably would need a try/catch here.

    Also, we don't need elseif, there's a return above.

  4. Coming from #3095893: Remove duplicate "add block" link from block content type view's "Results not found" message I was expecting to see something here for admin lists that are built with views, as in reality most content-entity list builders are overridden with a view. It would be good to get an issue summary update here to address what the intended course of action is there.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.