Content types are ordered by machine name on /node/add page (+ similar issues with other entities)

Maybe this?

#3 fixes it.

I like the idea. IMHO though we should adapt \Drupal\Core\Entity\Controller\EntityController::addPage as well as \Drupal\block_content\Controller\BlockContentController::add

On top of that I'm wondering whether the sorting belongs onto the theme layer or the controller

The second screenshot in #4 made me think, why is that also not wrong?
Because \Drupal\Core\Config\Entity\ConfigEntityListBuilder::load() does this:
uasort($entities, array($this->entityType->getClass(), 'sort'));

We should probably just use that.

If this were earlier days of D8, I'd add a , $sort = FALSE to loadMultiple, but it's too late for that.

If this were earlier days of D8, I'd add a , $sort = FALSE to loadMultiple, but it's too late for that.

Yeah in an ideal world we would not sort at all.

Issues get fixed in the dev branches. Please do not change that.

I agree we should use the Entity Type's sort method as mentioned in #8. See attached patch. No interdiff since it isn't based on #3.

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -76,8 +76,13 @@ public function addPage() {
   +    // See \Drupal\Core\Config\Entity\ConfigEntityBase::sort().
   

   I think this comment is redundant.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -76,8 +76,13 @@ public function addPage() {
   +    $node_types = $this->entityManager()->getStorage('node_type')->loadMultiple();
   ...
   +    uasort($node_types, array($this->entityManager()->getDefinition('node_type')->getClass(), 'sort'));
   +
   ...
          $access = $this->entityManager()->getAccessControlHandler('node')->createAccess($type->id(), NULL, [], TRUE);
   

   Might as well use entityTypeManager(). Could even assign to a local variable and reuse.

3. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -76,8 +76,13 @@ public function addPage() {
   +    uasort($node_types, array($this->entityManager()->getDefinition('node_type')->getClass(), 'sort'));
   

   use [] instead of array()

I think this comment is redundant.

I would agree and removed it. It was in \Drupal\Core\Config\Entity\ConfigEntityListBuilder::load(), which is why I copied it over in the first place.

Might as well use entityTypeManager(). Could even assign to a local variable and reuse.

Good point, though we're breaking consistency with the rest of the file, which uses entityManager() everywhere. I guess we can switch to it in this function. Thinking about switching away from EntityManager for the rest of the file is probably out of scope for this issue.

use [] instead of array()

I wanted to use array() for consistency, but I see now that [] is also used in some spots, so it isn't consistent anyway.

So the patch grew a bit for consistency withing the addPage-method, but it still does the same thing.

I'll have to read up on creating tests before I can include that as well.

Glad to see this issue taken up. Was about to post a patch I had made based on this post but thankfully I found this issue in the queue instead. I won't post my patch because the ones in this thread are already better tested.

Can't we can't just do this? (Re-posting attached from duplicate issue #2734573-2: Content types not sorted correctly on node/add page.) It's much simpler and it entirely unifies the code path between the two disparate pages. The node/add page is a list. Seems to make sense to use the list builder. :)

# 17(@TravisCarden) working fine and it is nice !!!!.

If you have 51 content types (it could happen!) this approach will fail.

This is because the list builders are paged, by default to 50 entries.

What? And there's no way to dynamically override it? Well, thanks for pointing that out, @tim.plunkett. I've created #2736377: Add ability to dynamically set limit on EntityListBuilder, which would allow us to do this:

$types = $this->entityTypeManager()
  ->getListBuilder('node_type')
  ->setLimit(FALSE)
  ->load();

But until such a method is available, perhaps we continue here with the previously discussed approaches.

It indeed feels like a weird implementation detail that the load method is actually public. Looking from far away a list builder should simply return the rendered list of stuff, potentially.

Modified patch #15 to apply against 8.3.x-dev.

Does block_content have the same issue? Could we fix both in own patch?

Before applying patch

After applying patch

Patch #24 works for both drupal 8.4.x and 8.5.x branch.

I've tested patch from #24 against 8.6.x and the patch works as expected.

As there are already screenshots by Mohit in #27 and the code has been updated from the comments from Tim Plunkett in #14, I'm going to set this as RTBC.

There is a question of if the issue gets postponed until #2736377: Add ability to dynamically set limit on EntityListBuilder gets fixed. There's also the question if a fix can be implemented for block_content at the same time (if it exists).

#7 and #26 haven't been addressed/answered

Also the 'Needs tests' tag added in #3 hasn't been addressed. I'd hate for this to get fixed then an accidental regression gets in unchecked later.

Based on #7 and #26 I'm moving this to the 'entity system' component.

I've tried to add unit tests for NodeController in the patch above.

A couple of comments:

1) NodeController gets UrlGenerator via dependency injection

The original code uses UrlGeneratorTrait. I changed the code because I couldn't figure out how to mock this part:

    if (count($content) == 1) {
      $type = array_shift($content);
      return $this->redirect('node.add', ['node_type' => $type->id()]);
    }

2) NodeControllerTest has helper class TestConfigEntityBase

I used the same approach as SearchPageRepositoryTest uses. A helper class which extends the original class which has the sort method. I couldn't figure out other way to mock.

Closed #2991874: Drupal Content sorted by machine name not title as a duplicate issue.

Closed #3007618: Allow Node Views to be sorted on the Node type label instead of the the Node type machine name as a duplicate issue.

I've been working on this some more lately. I have a patch coming up soon. My patch will address #7 and #26, and will also add tests for the all three controllers.

Please note that #3007618 has been re-opened, with a revamped title and summary. It describes a related, similar, issue in which Views results which are sorted by Content Type are ordered incorrectly by content type machine name rather than content type title.

This patch addresses #7 and #26. All three controllers get unit tests. NodeController and BlockContentController get functional tests. Also, I can create new issues if the tests are too extensive for this issue's scope.

(Edit: Details about the patch removed. I will add details after fixing the failing tests.)

This patch fixes the problems in the previous patch.

Summary

• Affects three "Add page" controllers: Node & Block Content & Entity
• When they show a list of bundles, the bundles are sorted using sort() method of the entity class
• Unit tests added for all three controller methods
• Functional tests added to assert the correct sort order (all three controllers)

Node

NodeControllerTest (unit tests)

• testAddPage() asserts: Access control + Render array content + Sort was called
• testAddPageRedirect() asserts: Redirect when only 1 bundle exists
• Mock class MockNodeType (to allow mocking static sort() method)
• Mock class MockRenderer (to allow asserting render array content)

NodeCreationTest (functional tests)

• testNodeAddPageOrder() asserts: Bundles are ordered by label, not id

Block Content

BlockContentController: Dependency injections refactored. Inject only themeHandler and urlGenerator. Removed injecting entityStorage because ControllerBase provides entityTypeManager already.

BlockContentControllerTest (unit test)

• testAddEmptyText() asserts: Render array content is the empty text when 0 bundles exist
• testAddForm() asserts: Render array content is the addForm when only 1 bundle exists
• testAddList() asserts: Render array content is the bundle list + Sort method was called
• Mock class MockBlockContentType (to allow mocking static sort() method)

BlockContentTestBase (functional tests)

• Method createBlockContentType() changed to allow giving $id and $label separately
• Usages of the above method updated (3 files in core)

BlockContentTypeTest (functional tests)

• testBlockContentAddPageOrder() asserts: Bundles are ordered by label, not id
• Fix $this->assertRaw('Bar', 'New name was displayed.') - It didn't work correctly because it matched "sidebar" css class

Entity

EntityController: Refactored because the actual entity objects must be loaded to use the sort() method. Previously the controller used EntityTypeBundleInfo which gives only entity IDs and labels.

EntityControllerTest (unit tests)

• testAddPageRenderArray() asserts: Access control + Render array content + Sort was called
• testAddPageRedirectNoBundles() asserts: Redirect when entity type doesn't support bundles
• testAddPageRedirectOneBundle() asserts: Redirect when only 1 bundle exists
• Mock class MockEntityType (to allow mocking static sort() method)
• Mock class MockRenderer (to allow asserting render array content)

EntityAddUITest (functional tests)

• testAddPageWithBundleEntities() added an assert that two bundles are in correct order
• testAddPageWithoutBundleEntities() refactored because the previous tests didn't make sense to me. The test scenario is supposed to test a situation where entity does not have bundles (based on its name), but it did add bundles and duplicated some of the asserts from testAddPageWithBundleEntities(). Also the main problem with the test scenario was the usages of entity_test_create_bundle() which did some kind of "shallow" addition of a bundle. The new bundle was found by EntityTypeBundleInfo but the definition couldn't be actually loaded (and they need to be loaded to be able to sort() them).

PS. Thanks @xano for helping with the questions I had while doing this patch.

@juhog: Could you please post a tests-only patch?

@cilefen Here's a tests-only patch for #42.

Tests-only patch for #42 again (incorrect diff root in the previous one).

The patch in #42 is full of rainbows and happiness. My Add Content page no longer looks like someone shook up all the content types in a Yahtzee cup and dumped them out at random. Thanks to all the contributors who have been working on this,

#42 passed all tests, so back to “needs review.”

It seems 8.7 has broken the patch. Quickly looking it looks like just the line numbers may be off.

Adjusted the patch so that it can apply with 8.7

fixed (tried to fix) the linting on 8.7

thanks @samuelmortenson for helping me out in slack :)

and rerolled for 8.8.x

hm. I think I didn't need to change the issue version, and probably could have uploaded both patches in the same comment. I wonder what 8.8.x should run though, maybe php 7.2 not sure which mysql.

This has tests, removing tag, until a review can give feedback on if it needs more :)

I thoroughly went through the last patch. Here is what I found:

1. About MockBlockContentType: This class was embedded at the end of the test file. In 1000's of test files, there is just one other *Test.php file containing more than one class. I went ahead and extracted MockBlockContentType, MockNodeType, and MockRenderer into separate files.
2. Removed the deprecated UrlGeneratorTrait from EntityController. Undone, see next patch.
3. Code style: I removed unnecessary brackets, removed unused use statements; formatted source code; fixed 32 coding standards messages.
4. I have slightly improved some documentation comments.
5. EntityControllerTest: Improved name of variable $content_entity_id to $bundle_entity_type_id.
6. EntityControllerTest: mockEntityType cannot work because we cannot mock a static function - see https://github.com/phpspec/prophecy/pull/20#issuecomment-18133965
7. EntityController: Made the diff slightly smaller by moving the $form_route_name line in front of if like it currently is in Core.
8. Reintroduced the fitting variable name $bundle_name instead of the technically correct, but less digestible $entity_type_id.
9. Moved uasort down to where we know that there are multiple bundles. I had to change the call counts in the tests as a result.
10. BlockContentTestBase::setUp: keep the block type label lowercase as it is currently in Core to prevent other people's tests from failing.
11. BlockContentTestBase::createBlockContentType: Use $id as default value for $label, so we have to change less lines (read: smaller diff).
12. assertUrl() accepts only one parameter - I have rewritten the message as a comment.
13. Made "instanceof EntityDescriptionInterface" ternary more friendly for PHPStan.
14. Fixed the tests locally. Well, let's see what the pipeline here on Drupal.org says.

Thanks to everyone on this issue for your enthusiasm in getting this fixed.

Changes since the last patch:

• Re-added UrlGeneratorTrait to EntityController because it broke tests.

Wow, but there‘s something wrong with patch #63...

I messed the last patch up and added a file "foo.patch" into the patch 😕. Here comes my next attempt to get the tests green. Unfortunately, I could not reproduce the test failures locally. I wonder if the "foo.patch" confused the test runner?

My changes since the last patch:

1. I removed the foo.patch.
2. Fixed function signature of MockEntityType::sort to match that of the parent.
3. I simplified code in MockRenderer: in_array was not needed for freshly initialized variable.
4. I fixed remaining 5 coding standards messages.

Changes since the last patch:

1. NodeCreationTest: Use entityTypeManager instead of deprecated entityManager. This should get the failed tests green.
2. BlockContentWizardTest: Removed unnecessary line change from diff.
3. Tests relied on the SIMPLETEST_BASE_URL being exactly 'http://localhost', which defeats the purpose of the variable.
4. NodeController: Removed unused $key from foreach loop.

Rerolling against the last 8.8.x branch changes

Thank you everyone for working on this. :D

I've test against 8.8.x and the bundles are ordered alphabetically on the "Add content" page.

I've checked the code and it all looks good and there are new tests included.

So, I'm going to set this to RTBC!

I'm reverting the issue status change made by test bot a couple of days ago. It seems like an unrelated random failure. At the moment the last patch has ran tests successfully two times.

Looks good, but there is a regression here, we're dropping support for the two hooks that allow bundle definition - hook_entity_bundle_info and hook_entity_bundle_info_alter

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -110,63 +110,81 @@ public static function create(ContainerInterface $container) {
   +    // Redirect if there are no bundles.
   +    if (!$bundle_entity_type_id) {
   +      $form_route_name = 'entity.' . $entity_type_id . '.add_form';
   +      $form_route_param_key = $entity_type->getKey('bundle');
   +      $bundle_name = $entity_type_id;
   +      return $this->redirect($form_route_name, [$form_route_param_key => $bundle_name]);
   +    }
   

   We can't be sure this route exists or that the user has access. Are we sure this change is in scope? Even still, I don't think the code is doing what the comment says.

   'Redirect if there are no bundles' implies we're checking that no bundles have been created or that there are none defined in the entity bundle info, but this condition is testing if the entity has a bundle entity.

   I think we're confusing 'has an entity for bundles' with 'there are bundles' and 'supports bundles'.

   i.e. just because an entity doesn't have a bundle entity type, doesn't mean there aren't multiple bundles, because there are other ways (besides config entities) to define bundles for an entity type.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -110,63 +110,81 @@ public static function create(ContainerInterface $container) {
   +    $bundle_entity_type_storage = $this->entityTypeManager->getStorage($bundle_entity_type_id);
   +    /** @var \Drupal\Core\Entity\EntityInterface[] $bundle_entities */
   +    $bundle_entities = $bundle_entity_type_storage->loadMultiple();
   

   we need to use the entity bundle info service here, not the entity type storage, config entities are not the only way to define bundles of an entity type, there are hooks -eg hook_entity_bundle_info and hook_entity_bundle_info_alter

   also, we don't have test coverage for those scenarios

3. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -110,63 +110,81 @@ public static function create(ContainerInterface $container) {
   +    uasort($bundle_entities, [$bundle_entity_type->getClass(), 'sort']);
   

   we can't use this with bundles that are defined in the hooks (as they're not guaranteed to be entities), so we'll need to add back the other values provided by the hooks after we sort here

4. +++ b/core/modules/block_content/src/Controller/BlockContentController.php
   @@ -102,7 +112,8 @@ public function add(Request $request) {
   -    $block = $this->blockContentStorage->create([
   +    $manager = $this->entityTypeManager();
   +    $block = $manager->getStorage('block_content')->create([
   

   out of scope?

5. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTestBase.php
   @@ -99,10 +101,10 @@ protected function createBlockContent($title = FALSE, $bundle = 'basic', $save =
   -  protected function createBlockContentType($label, $create_body = FALSE) {
   +  protected function createBlockContentType($id, $label = '', $create_body = FALSE) {
   

   this is an api change, we should be adding a new method instead of modifying the argument order of the existing one - because its a base class, its considered API

6. +++ b/core/modules/block_content/tests/src/Functional/Views/BlockContentWizardTest.php
   @@ -37,6 +37,7 @@ public function testViewAddBlockContent() {
   +    /** @var \Drupal\Core\Entity\EntityStorageInterface $view_storage_controller */
   

   out of scope?

Please note that the sorting of config entities having labels with specific characters like "É" won't work properly as they will appear at the end of the list.

.

How did we go from a 1kb patch to almost 60kb :O Are we still on track, or are we doing unrelated changes?

Could we leave the sorting up to the entity class, i.e. run uasort($node_types, [$definition->getClass(), 'sort']);?

"How did we go from a 1kb patch to almost 60kb"

Scope was extended to include NodeController, EntityController, and BlockContentController (#7 and #26). Also tests for all of those.

The current newest patch has some issues as pointed out in #71. One important thing that I personally missed while writing one of these patches is that bundles can be defined in multiple ways, not just config entities.

The problem with uasort($node_types, [$definition->getClass(), 'sort']) is the one mentioned in #71 point number 3.

In a minute, I'll go ahead and work on the concerns raised above. First, however, I will upload an unmodified patch. It only differs from comment #67 in that it cleanly applies to Drupal Core 8.9.x.

Patch from #77 applies cleanly on 8.8.2 as well, while #67 does not.

The patch #77 is working correctly on 8.8.5

Can someone confirm which Drupal version will incorporate this patch

@drupalisedmanish It will have to be reviewed and tested by the community to be incorporated in a release.

Re-Roll of #77 for 8.9.x as of commit 8ce62cd71f

Rerolled for 9.1.x.

working on it

Solving test cases.

Sorry .I am stuck in another task which will take more time so i am un-assign this issue

Declaring ::setUp without a void return typehint

Added #2863292: Cannot manually set the sorting of content types on page /node/create as related and will close that as a duplicate, also added credit for leisurman who reported that issue.

Working on this. Needs a re-roll.

Here I have re-rolled patch #92 and tried to fix failed test of patch #92.

Solving failed test case. Kindly follow a new patch.

I've been working on the problems pointed out in #71. I'm hoping get the patch ready during the weekend. The patch will also address some problems in the unit tests.

Thank you larowlan for the review and insights given on comment #71!

This patch is basically a complete rewrite of the changes needed in this issue.

Summary:

• Fixed regression issue related to EntityController bundle info
• Unit testing approach improved by implementing the idea described in Unit testing more complicated Drupal classes doc page
• Functional test fixes and improvements

EntityController

Previous approach was based on some misunderstandings and caused regression issues mentioned in #71. This new solution avoids making any major changes to the addPage() method. The addPage() method gets the bundles from bundle info service.

If I've understood correctly, a content entity will get all of its bundles from just one source: the bundle entity types or the bundle info hooks. If a content entity were able to get bundles from both sources, I believe the addPage() would break when loadBundleDescriptions() method is called.

When bundles are defined by a bundle entity type, they are now being sorted with a new method sortBundleEntities(). The actual sorting is done by calling the static 'sort' method on the bundle entity type class (usually ConfigEntityBase). Having this wrapper method makes unit testing easier.

When bundles are defined without a bundle entity type, the controller doesn't do any sorting. Sorting doesn't seem necessary because the bundle info hooks allow the implementation to place the bundles in which ever order they want.

Other minor changes:

• url_generator service injected to make unit testing easier
• "add_bundle_message" simplified to make unit testing easier

BlockContentController

New method sortBlockContentTypes() handles sorting the bundles. This is very similar to the new sort method in EntityController.

Minor change: url_generator service injected to make unit testing easier.

NodeController

New method sortNodeTypes() handles sorting the bundles. This is very similar to the new sort method in EntityController.

New method redirectNodeAddPage() to make unit testing easier.

Unit tests: BlockContentControllerTest

Static 'sort' method calls are the main problem to solve in these unit tests. Previous solution tried to handle those by mocking the block content type class.

This patch changes the approach completely and implements the idea described in Unit testing more complicated Drupal classes doc page. New class TestBlockContentController extends the BlockContentController and overrides the sortBlockContentTypes() method with a custom logic that doesn't require calling static methods. This method override also allows us to count the number of times the actual sort function is called. The way I see it, BlockContentController doesn't actually care how the bundles are sorted, it just wants the block content type class to do some sorting.

Other minor changes:

• TestBlockContentController also overrides addForm() method to make unit testing easier. More notes in the method docblock.
• New helper method createBlockContentTypeDouble() to avoid repetition in each test case.
• New assertSame() for asserting the order of the bundles (render array assertEquals() doesn't do that)

Side note: When only one block content type exists, would be nice to do a redirect, instead of printing out a form.

Unit tests: NodeControllerTest

In general, the problems and the solutions in this class are very similar to the ones in BlockContentControllerTest. In summary:

• New class TestNodeController, overrides sortNodeTypes() and redirectNodeAddPage()
• New helper method createNodeTypeDouble() to avoid repetition in each test case.
• New assertSame() for asserting the order of the bundles (render array assertEquals() doesn't do that)
• renderer service is mocked with PHPUnit (instead of Prophecy) to allow mocking a method that uses an argument by reference

Unit tests: EntityControllerTest

In general, the problems and the solutions in this class are very similar to the ones in BlockContentControllerTest. In summary:

• New class TestEntityController, overrides sortBundleEntities()
• New helper method createBundleDouble() to avoid repetition in each test case.
• New assertSame() for asserting the order of the bundles (render array assertEquals() doesn't do that)
• renderer service is mocked with PHPUnit (instead of Prophecy) to allow mocking a method that uses an argument by reference

EntityController has quite a lot of different cases though because the bundles can be defined either by a bundle entity or by bundle info hooks.

This file covers the following cases:

• Bundles are defined by a bundle entity type, and zero bundles exist
• Bundles are defined by a bundle entity type, and one bundle exists
• Bundles are defined by a bundle entity type, and multiple bundles exist
• Bundles are defined by bundle info hooks, and zero bundles exist
• Bundles are defined by bundle info hooks, and one bundle exists
• Bundles are defined by bundle info hooks, and multiple bundles exist

Functional tests: EntityAddUITest

Functional test case testAddPageWithBundleEntities() fixed so that it passes only when the bundles are actually sorted by label.

Test case testAddPageWithoutBundleEntities() has been reverted to its original form. The changes in the previous patches were invalid, due to some misunderstanding.

Out-of-scope: Bundle IDs and labels edited to keep them consistent throughout the test case.

Functional tests: BlockContentTypeTest

New functional test case testBlockContentAddPageOrder(). This test needs to define both the ID and the label of the test block content types it creates. A new method createBlockContentTypeWithId() has been added to BlockContentTestBase to allow creating those block content types.

Out-of-scope: One assertRaw() usage fixed. See #42 for details.

Functional tests: NodeCreationTest

New functional test case testNodeAddPageOrder().

Other notes

I've used "@noinspection PhpUndefinedMethodInspection" in the unit test files. Prophecy objects tend to trigger a lot of these IDE inspection warnings. I will remove these if this isn't the proper way to deal with the warnings.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -119,7 +125,8 @@ public static function create(ContainerInterface $container) {
   -    return new RedirectResponse(Url::fromRoute($route_name, $route_parameters, $options)->toString(), $status);
   +    $url = $this->urlGenerator->generateFromRoute($route_name, $route_parameters, $options);
   +    return new RedirectResponse($url, $status);
   

   Why this change? It's not clear how this is related to fixing the bug.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -143,6 +150,7 @@ public function addPage($entity_type_id) {
        ];
   +
        if ($bundle_entity_type_id) {
   
   @@ -150,12 +158,12 @@ public function addPage($entity_type_id) {
          ]);
   +
          // Filter out the bundles the user doesn't have access to.
   
   @@ -165,20 +173,23 @@ public function addPage($entity_type_id) {
        $form_route_name = 'entity.' . $entity_type_id . '.add_form';
   +
        // Redirect if there's only one bundle available.
   ...
        }
   +
        // Prepare the #bundles array for the template.
   

   Out of scope

3. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -150,12 +158,12 @@ public function addPage($entity_type_id) {
   -        '@add_link' => Link::createFromRoute($link_text, $link_route_name)->toString(),
   +        ':url' => $this->urlGenerator->generateFromRoute($link_route_name),
   

   Same question

4. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -165,20 +173,23 @@ public function addPage($entity_type_id) {
   -      // Add descriptions from the bundle entities.
   +
          $bundles = $this->loadBundleDescriptions($bundles, $bundle_entity_type);
   

   Why remove this?

5. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -345,4 +356,37 @@ protected function loadBundleDescriptions(array $bundles, EntityTypeInterface $b
   +   * @param array $bundles
   +   *   An array of bundle information.
   

   What kind of information? Machine names? Objects? This should be something like string[]

6. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -345,4 +356,37 @@ protected function loadBundleDescriptions(array $bundles, EntityTypeInterface $b
   +    uksort($bundles, function ($a, $b) use ($storage, $bundle_entity_type_class) {
   +      $bundle_a = $storage->load($a);
   +      $bundle_b = $storage->load($b);
   

   So the load is done on every sort comparison? That seems unnecessary. Why not load them all, key by machine name, sort the objects, then return the array keys?

Stopping my review here because the rest of the patch is more of the same confusing Url/urlGenerator switches.

The changes in the previous patch that aren't directly related to sorting the bundles are related to making things easier in the unit tests. Apologies for any confusion caused.

The three controllers related to this issue currently don't have any unit tests. I suppose the options are as follows:

1. Do not add any unit tests in this issue.
2. Add unit tests but only for situations that list some bundles, and assert only the order of those bundles.
3. Add unit tests for each "add page", but don't try to fix any problems that are making unit testing complicated.
4. Add unit tests for each "add page", and try to include some minor fixes to make things easier in the unit tests.

The previous patch is aiming for option 4.

Which option or approach would be preferred? I can modify the proposed solution accordingly.

Regarding the specific comments:

#105.1 - The motivation has been to make unit testing easier. I will look into reverting these kinds of changes, moving towards option 3 mentioned above.

#105.2 - Noted, next patch will revert this and other similar out of scope changes.

#105.3 - "add_bundle_message" simplified to make unit testing easier. I'm trying to avoid having to use Link::createFromRoute() inside the unit tests. I've seen that approach in some other core unit tests though. If that solution (or some other solution) is preferred, I will revert this change.

#105.4 - Noted, next patch will revert this and other similar out of scope changes. The comment seemed unnecessary because the method name is very descriptive already.

#105.5 - Noted, next patch will try to change the method to use a more simple parameter. In the current solution I tried to follow the example given by loadBundleDescriptions() method in the same class. This new method takes the same parameter. It's the bundle information returned from EntityBundleInfo::getBundleInfo().

#105.6 - Noted, next patch will implement the suggestion.

Thank you for the review tim.plunkett, the insights are much appreciated.

This patch addresses the feedback given in comment #105.

Summary

• EntityController::sortBundleNames() - sorts bundle names
• NodeController::getNodeTypes() - loads bundles and sorts them
• BlockContentController::getBlockContentTypes() - loads bundles and sorts them
• All unit tests removed
• All unit testing related changes removed
• All out of scope changes removed

EntityController:

New method sortBundleNames() implements the idea suggested in #105.6: Receives bundle names string[], and returns sorted bundles names string[].

However, all the logic in addPage() is currently assuming the data is in bundle information array format. So, some more processing is needed at addPage() to sort the bundle information array with the results given by sortBundleNames(). If we want to avoid this, we could give the bundle information array as a parameter to our custom sortBundleNames() method. Or addPage() could be refactored to not rely on the bundle information array format so much.

When bundles are defined without a bundle entity type, the controller doesn't do any sorting. Sorting doesn't seem necessary because the bundle info hooks allow the implementation to place the bundles in which ever order they want.

NodeController and BlockContentController:

Both controllers apply the same new logic. Bundles are provided by a new method: getNodeTypes() / getBlockContentTypes(). The new method handles loading the bundles and sorting them with the sort() method of the bundle entity type definition class.

Functional tests:

All three controllers have functional tests for checking the correct bundle sort order.

Unit tests:

This patch removes all the unit tests that were being suggested in previous patches. I'm assuming it's better to leave the unit testing to some other issue due to the reasons mentioned in comment #106.

If unit tests are eventually added in some issue, patch #104 can be a good starting point since it provides solutions for many mocking related problems in these three controllers (NodeController, BlockContentController, EntityController).

Tested the latest patch on simplytest.me
Worked nicely
Able to see content types alphabetically both on node/add screen and content type listing page
As well as dropdowns
This can be moved for RTBC

@samiullah thanks for testing! It isn't clear from your comment/screenshots how you confirmed that the content types are not sorted by machine name?

Are you able to update your comment with steps to reproduce and verify that the order is alpha by content type name rather than machine name?

Also the patch will need a code review before it can be marked RTBC.

@pameeela Thanks for feedback

Following are the steps to manually test this one

1. Install fresh instance of core 9.1 x branch and add patch file link on simplytest.me
2 Wait for the site to spin up
3. Login as admin (uname: admin pwd: admin)
4. By default we have two content types: Article and Basic Page
5. Machine name of Article is article and for Basic Page it is page
6. Add new content type example Zebra machine name would be zebra
7. Edit the Article content type and change Content type name to ZArticle, note that this step will not change the machine name
8. Check the content type listing and see if they are appearing alphabetically by name
9. Navigate to node/add page and observe if same order of content type is available here or not
10. One more test u can do is to rename zebra content type to example : Apple and see if it gets sorted by name or not

@pameeela yes this one needs to Code reviewed as well

I have carefully reviewed @juhog's patch from #107. From my perspective, the code looks wonderful. I didn't see any code style violation or out-of-scope change. My manual tests based on 9.2.x-dev + the patch didn't reveal any problems. PhpStorm inspections are clean. Thanks @juhog!

From my perspective, this is ready to be commited.

This is a nice change! RTBC++.

It will also allow contrib to add a weight sort to other entity types, such as node_type. This small chunk of code adds the functionality, without adding the UI that will also be desired:

/**
 * Implements hook_entity_type_alter().
 */
function mymodule_entity_type_alter(array &$entity_types) {
  if (!isset($entity_types['node_type'])) {
    return;
  }
  /** @var \Drupal\Core\Entity\EntityTypeInterface $entity_type */
  $entity_type = $entity_types['node_type'];

  // Add 'weight' to the entity keys.
  $entity_keys = $entity_type->get('entity_keys');
  $entity_keys['weight'] = 'weight';
  $entity_type->set('entity_keys', $entity_keys);

  // Add 'weight' to the config export.
  $config_export = $entity_type->get('config_export');
  $config_export[] = 'weight';
  $entity_type->set('config_export', $config_export);
}

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -167,6 +167,11 @@ public function addPage($entity_type_id) {
          // Add descriptions from the bundle entities.
          $bundles = $this->loadBundleDescriptions($bundles, $bundle_entity_type);
   +
   +      $bundle_names_unsorted = array_keys($bundles);
   +      $bundle_names_sorted = $this->sortBundleNames($bundle_names_unsorted, $bundle_entity_type);
   +      $bundle_names_sorted_as_keys = array_flip($bundle_names_sorted);
   +      $bundles = array_replace($bundle_names_sorted_as_keys, $bundles);
        }
   

   We have bundles here.

   We then convert it into an array of bundle hames (although is it not bundle machine names at this point?).

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -345,4 +350,35 @@ protected function loadBundleDescriptions(array $bundles, EntityTypeInterface $b
   +    }
   +
   +    $bundle_entity_storage = $this->entityTypeManager->getStorage($bundle_entity_type->id());
   +    $bundle_entities = $bundle_entity_storage->loadMultiple($bundle_names);
   +    uasort($bundle_entities, function ($a, $b) use ($bundle_entity_type_class) {
   +      return forward_static_call_array([$bundle_entity_type_class, 'sort'], [$a, $b]);
   +    });
   

   Then here, we're loading bundles again to sort them.

   Why not a ::sortBundles() method without having to deconstruct then reconstruct the array? The block and node controllers appear to be doing this already. If we can't do that, I think we need to explain why we're doing all the array_flip/array_replace dance to avoid it - but can't see why.

Thank you for the comments @catch, let me elaborate:

We have bundles here.

We then convert it into an array of bundle hames (although is it not bundle machine names at this point?).

We have bundle information array and it has very minimal data. The actual bundle entity type classes are not available.

Then here, we're loading bundles again to sort them.

Bundle classes are being loaded for the first time. Loading is necessary to be able to use the sort() method of the bundle entity type definition class.

Why not a ::sortBundles() method without having to deconstruct then reconstruct the array? The block and node controllers appear to be doing this already. If we can't do that, I think we need to explain why we're doing all the array_flip/array_replace dance to avoid it - but can't see why.

Node and Block use an bundle entity type to define the bundles. The situation in this method isn't the same.

The current approach for the sortBundleNames() method was suggested in #105 points 5 & 6 by tim.plunkett. I mentioned some more details in comment #107:

"Some more processing is needed at addPage() to sort the bundle information array with the results given by sortBundleNames(). If we want to avoid this, we could give the bundle information array as a parameter to our custom sortBundleNames() method. Or addPage() could be refactored to not rely on the bundle information array format so much."

In summary:

A decision needs to be made about what kind of parameter should the "sort" method receive. Orignal solution on #104 was to give the bundle information array as parameter (the same way as loadBundleDescriptions() does). This was changed in #107 to be just a string[]. From my point of view, the options are limited, and each option has it's downsides, unless some overall refactoring is done for the addPage() method.

If the current approach seems valid, I'm happy to add a code comment explaining the need for this solution.

We have bundle information array and it has very minimal data. The actual bundle entity type classes are not available.

Ahh OK good point. However that brings up another problem I think. It's possible to define bundles entirely in code without using bundle entities at all, and the generic method is assuming that bundle entities exist. This is fine for the entity test module since it uses bundle entities, but it's not necessarily going to be for custom entity types. Given we don't have a case in core apart from entity test module where we can use it, should be adding the generic method at all? Would be tempted to fix node and block here, and move the generic method to a follow-up.

It's possible to define bundles entirely in code without using bundle entities at all, and the generic method is assuming that bundle entities exist.

I'm not sure if I understood correctly. The "generic method" refers to EntityController::addPage()?

EntityController::addPage() does currently work in both situations. The method checks whether or not the current entity type is using a bundle entity type.

If bundle entity type is being used, the method checks which bundles are accessible, adds bundle descriptions, and with patch #107 sorts those bundles.

If there is no bundle entity type, the method doesn't do any of those things and just lists the bundles in the order that bundle information array has them. The addPage() method could apply some sorting in this situation as well but it seems unnecessary because the bundle info hooks allow the implementation to place the bundles in which ever order they want.

Would be tempted to fix node and block here, and move the generic method to a follow-up.

Node and Block Content are indeed more simple situations than the situation with EntityController.

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
@@ -345,4 +350,35 @@ protected function loadBundleDescriptions(array $bundles, EntityTypeInterface $b
+      return forward_static_call_array([$bundle_entity_type_class, 'sort'], [$a, $b]);

Why are we using forward_static_call_array() and not call_user_func_array()? It doesn't seem necessary as far as I can see. (aside - never seen forward_static_call_array() before - neat).

If it is necessary then a comment as to why would be great.

Re-rolled patch.

I also changed:
return forward_static_call_array([$bundle_entity_type_class, 'sort'], [$a, $b]);
to
return call_user_func_array([$bundle_entity_type_class, 'sort'], [$a, $b]);

I changed the test cases.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -167,6 +167,11 @@ public function addPage($entity_type_id) {
   +      $bundle_names_unsorted = array_keys($bundles);
   +      $bundle_names_sorted = $this->sortBundleNames($bundle_names_unsorted, $bundle_entity_type);
   +      $bundle_names_sorted_as_keys = array_flip($bundle_names_sorted);
   

   This is a slightly wordy set of variable names, maybe it explains what's going on but usually I think we would just say something shorter e.g:

   $bundle_names = $this->sortBundleNames(array_keys($bundles), $bundle_entity_type);
   $bundles = array_replace(array_flip($bundle_names), $bundles);
   

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -345,4 +350,35 @@ protected function loadBundleDescriptions(array $bundles, EntityTypeInterface $b
   +    uasort($bundle_entities, function ($a, $b) use ($bundle_entity_type_class) {
   +      return call_user_func_array([$bundle_entity_type_class, 'sort'], [$a, $b]);
   +    });
   

   Why do we even need the anonymous function here?

      uasort($bundle_entities, [$bundle_entity_type_class, 'sort']);
   

   would seem to be equivalent?

3. +++ b/core/modules/block_content/src/Controller/BlockContentController.php
   @@ -89,6 +89,23 @@ public function add(Request $request) {
   +    $block_content_types = $this->blockContentTypeStorage->loadMultiple();
   +    $definition = $this->entityTypeManager()->getDefinition('block_content_type');
   

   Seems a bit odd that we have the storage injected (for which we need the entity type manager already) but not the definition, maybe not worth fixing here though. Looking at this I wonder if we should try to unify the node and block content controllers so they extend from EntityController instead, maybe there is an issue for this already?

Applied patch #124 and it works fine.Adding screenshots below.

Before patch:

After patch:

This patch addresses the following comments:

#116.2

I think we need to explain why we're doing all the array_flip/array_replace dance to avoid it

Code comment added. Another option would be to change the sort method, more details in #117.

#125.1

This is a slightly wordy set of variable names

Variables have been modified as suggested.

#125.2

Why do we even need the anonymous function here?

Modified as suggested. Indeed anonymous function seems unnecessary.

I have tested the patch locally and read the full patch again. Looks good to me 🙂

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -345,4 +352,33 @@ protected function loadBundleDescriptions(array $bundles, EntityTypeInterface $b
   +  protected function sortBundleNames(array $bundle_names, EntityTypeInterface $bundle_entity_type) {
   +    $bundle_entity_type_class = $bundle_entity_type->getClass();
   +    if (!is_callable([$bundle_entity_type_class, 'sort'])) {
   +      return $bundle_names;
   +    }
   +
   +    $bundle_entity_storage = $this->entityTypeManager->getStorage($bundle_entity_type->id());
   +    $bundle_entities = $bundle_entity_storage->loadMultiple($bundle_names);
   +    uasort($bundle_entities, [$bundle_entity_type_class, 'sort']);
   +
   +    return array_keys($bundle_entities);
   +  }
   

   Not doing this as part of loadBundleDescriptions means we have to load the bundles twice which feels wasteful. It's tempting to make loadBundleDescriptions return the sorted list? If it did then contrib/custom usages would be fixed - see http://grep.xnddx.ru/search?text=loadBundleDescriptions&filename= But that has the downside of then becoming a really bad method name.

   I wish \Drupal\Core\Entity\Controller\EntityController::addPage() was being dealt with in a follow-up issue. There are a number of things in it that make it tricky to handle. It has to deal with non config entity bundles and it already has a bad code flow (it does a redirect after doing unnecessary work if there is only 1 bundle). I think it would make it much easier to pull this part of the change and do it separately. Especially since the search of contrib shows that the current solution means that we need a CR that details changes contrib should make.

2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTestBase.php
   @@ -112,4 +112,27 @@ protected function createBlockContentType($label, $create_body = FALSE) {
   +  protected function createBlockContentTypeWithId($id, $label = '') {
   

   This should accept an array of values to be more flexible - like createContentType....

   Ahh I see we already have \Drupal\Tests\block_content\Functional\BlockContentTestBase::createBlockContentType... I think we should overload the first argument $label and change it to $values - if it is a string treat it as the label otherwise treat it as an array of values. And then in a follow-up we should deprecate the string code path. So we can typehint with an array in Drupal 10.

Addressed comment #132

Correct patch and interdiff between patch #127 and #138.

+++ b/core/modules/block_content/tests/src/Functional/BlockContentTestBase.php
@@ -91,7 +91,7 @@ protected function createBlockContent($title = FALSE, $bundle = 'basic', $save =
+   * @param array|string $values
    *   The block type label.

@@ -99,12 +99,17 @@ protected function createBlockContent($title = FALSE, $bundle = 'basic', $save =
+  protected function createBlockContentType($values, $create_body = FALSE) {
+    if (is_array($values)) {
+      $bundle = BlockContentType::create($values);
+    }
+    else {
+      $bundle = BlockContentType::create([
+        'id' => $values,
+        'label' => $values,
+        'revision' => FALSE,
+      ]);
+    }

The documentation needs to change to explain what the array is...

For example

   *   An array of settings to change from the defaults.
   *   Example: 'label' => 'foo'.

Plus we have to cope where id is not set in $values - see \Drupal\block_content\Tests\Views\BlockContentTestBase::createBlockContentType - for how to do this. Eventually we'll move this into a test trait and not have the duplicate code.

I attached a new patch that address the comment #139 and interdiff.

Applied patch #141 and it looks good. Screenshots below.

before patch

after patch

I think we can make the change safer by not adding new protected methods that don't really do much anyway.

1. +++ b/core/modules/block_content/src/Controller/BlockContentController.php
   @@ -73,7 +73,7 @@ public function __construct(EntityStorageInterface $block_content_storage, Entit
   -    $types = $this->blockContentTypeStorage->loadMultiple();
   +    $types = $this->getBlockContentTypes();
   
   @@ -89,6 +89,23 @@ public function add(Request $request) {
   +  /**
   +   * Gets block content types.
   +   *
   +   * Gets all block content types and sorts them with the sort() method of the
   +   * block content type definition class.
   +   *
   +   * @return \Drupal\block_content\BlockContentTypeInterface[]
   +   *   A sorted array of block content types.
   +   */
   +  protected function getBlockContentTypes() {
   +    /** @var \Drupal\block_content\BlockContentTypeInterface[] $block_content_types */
   +    $block_content_types = $this->blockContentTypeStorage->loadMultiple();
   +    $definition = $this->entityTypeManager()->getDefinition('block_content_type');
   +    uasort($block_content_types, [$definition->getClass(), 'sort']);
   +    return $block_content_types;
   +  }
   

   There's no need for the new protected method.

   $types = $this->blockContentTypeStorage->loadMultiple();
   uasort($types, [$this->blockContentTypeStorage->getEntityType()->getClass(), 'sort']);
   

   Not adding a protected method makes this a safer change to make.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -90,7 +90,7 @@ public function addPage() {
   -    foreach ($this->entityTypeManager()->getStorage('node_type')->loadMultiple() as $type) {
   +    foreach ($this->getNodeTypes() as $type) {
   
   @@ -109,6 +109,23 @@ public function addPage() {
   +  /**
   +   * Gets node types.
   +   *
   +   * Gets all node types and sorts them with the sort() method of the node type
   +   * definition class.
   +   *
   +   * @return \Drupal\node\NodeTypeInterface[]
   +   *   A sorted array of node types.
   +   */
   +  protected function getNodeTypes() {
   +    /** @var \Drupal\node\NodeTypeInterface[] $node_types */
   +    $node_types = $this->entityTypeManager()->getStorage('node_type')->loadMultiple();
   +    $definition = $this->entityTypeManager()->getDefinition('node_type');
   +    uasort($node_types, [$definition->getClass(), 'sort']);
   +    return $node_types;
   +  }
   

   There's no need for the new protected method.

     public function addPage() {
       $definition = $this->entityTypeManager()->getDefinition('node_type');
       $build = [
         '#theme' => 'node_add_list',
         '#cache' => [
           'tags' => $definition->getListCacheTags(),
         ],
       ];
   
       $content = [];
   
       $types = $this->entityTypeManager()->getStorage('node_type')->loadMultiple();
       uasort($types, [$definition->getClass(), 'sort']);
       // Only use node types the user has access to.
       foreach ($types as $type) {
         // ...
   

Hi, thank you @alexpott
I have made the suggested changes. Kindly review.

+++ b/core/modules/block_content/src/Controller/BlockContentController.php
@@ -74,6 +74,7 @@ public function __construct(EntityStorageInterface $block_content_storage, Entit
     $types = $this->blockContentTypeStorage->loadMultiple();
+    uasort($types, [$this->blockContentTypeStorage->getEntityType()->getClass(), 'sort']);

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -80,6 +80,7 @@ public static function create(ContainerInterface $container) {
+    $definition = $this->entityTypeManager()->getDefinition('node_type');

@@ -89,8 +90,10 @@ public function addPage() {
+    uasort($types, [$definition->getClass(), 'sort']);

This feels inconsistent now, we should be getting the entity type definition class the same way in both cases I think? At least in the node case, the code that gets the definition could be closer to where it is actually used.

@longwave $this->entityTypeManager()->getDefinition('node_type') and $node_storage->getEntityType() returned different information then we'd have a tonne of problems. The issue is the NodeController doesn't inject the storage and we shouldn't be changing that here. Imo getting the entity type in the most convenient way in either class is fine.

Fair enough, it does feel weird doing it two different ways in one patch but I guess it doesn't matter really and I agree fixing NodeController is out of scope.

Otherwise all points addressed and this patch has got much smaller and more focused so I think it is RTBC.

Created the follow-up for the de-scoped work - #3196798: Fix EntityController::addPage so bundles are sorted by label and not ID

Committed and pushed f0eaa97e13 to 9.2.x and bd41f89e84 to 9.1.x. Thanks!

Backported to 9.1.x since now this change only adds sorts inline to existing methods.

I came here from the release notes for 9.1.x. It is a little confusing: I think the issue was re-scoped (and I see a follow-up issue) but the title and issue summary were not updated.