Problem/Motivation

Found here https://git.drupalcode.org/project/drupal/-/merge_requests/12445#note_55...

EntityListBuilder::getDefaultOperations is protected, but the ConfigEntityListBuilder subclass and all its sub classes override that visibility to public. This seems like an accident when this was initially added in https://git.drupalcode.org/project/drupal/-/commit/b504423ed07e9bb437e96...

Proposed resolution

Change all methods to protected somehow (BC?)

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3538658

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

acbramley created an issue. See original summary.

libbna’s picture

Assigned: Unassigned » libbna

I will give this a try!

libbna’s picture

Assigned: libbna » Unassigned
Status: Active » Needs review

I’ve only changed the getDefaultOperations() method of the ConfigEntityListBuilder class to protected. Is there any other place where this change needs to be reflected or updated?

godotislate’s picture

core/modules/block/src/BlockListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/block_content/src/BlockContentTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/comment/src/CommentTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/field_ui/src/FieldConfigListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/filter/src/FilterFormatListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/image/src/ImageStyleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/menu_ui/src/MenuListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/node/src/NodeTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/responsive_image/src/ResponsiveImageStyleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/search/src/SearchPageListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/shortcut/src/ShortcutSetListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/taxonomy/src/VocabularyListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/user/src/RoleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/views_ui/src/ViewListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
godotislate’s picture

Oh, and a couple content entity list builders override as public as well:

core/modules/menu_link_content/src/MenuLinkListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/menu_link_content/src/MenuLinkListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
godotislate’s picture

I wonder that since these methods are public now, we can't just change them to protected right away, but need to trigger deprecations and add a CR that they will be changed to protected in D12.

acbramley’s picture

Status: Needs review » Needs work
libbna’s picture

Hi @godotislate thanks for providing the whole list. So instead of changing it to protected should I add deprecated message?

berdir’s picture

I'm not sure how the deprecation message would work. We'd need the differentiate the between internal and external calls, that sounds complicated and slow (checking backtrace?).

These were never meant to be called directly and doing so would be result in incomplete and broken results. Making it protected won't break subclasses, it's possible to extend the visibility of a protected method in the parent class.

I can't find any calls that aren't $this->getDefautOperations(), I'd say we either just do it or we don't bother and leave it?

godotislate’s picture

@libbna sounds like you can go ahead and change all those methods to protected per #10.

libbna’s picture

Assigned: Unassigned » libbna

Sure. Thanks! I will do it EOD.

libbna’s picture

Assigned: libbna » Unassigned
Status: Needs work » Needs review
godotislate’s picture

Status: Needs review » Reviewed & tested by the community

There was test failure, so I rebased. Test pass now, so this LGTM.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

godotislate’s picture

Status: Needs work » Needs review

Rebased.

acbramley’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

astonvictor’s picture

seems like all methods were updated
+1 RTBC

  • catch committed 513cbded on 11.x
    task: #3538658 getDefaultOperations() in ConfigEntityListBuilder and sub...
catch’s picture

Status: Reviewed & tested by the community » Fixed

I think this is fine - the chances of someone calling these are very, very small. However just in case I've only committed it to the 11.x branch (will become 11.4.x) so there's plenty of time to discover if someone somehow is doing that.

Committed/pushed to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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