Problem/Motivation

The two DefaultTableMapping::setFieldNames() and ::setExtraColumns() methods are internal helpers of the table mapping class and should not be exposed as public API.

Proposed resolution

Change them to be protected instead.

Remaining tasks

Wait until the 9.0.x branch is opened, write a patch, etc.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

Release notes snippet

Nope.

Issue fork drupal-3067336

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:

About issue forks

Comments

amateescu created an issue. See original summary.

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Postponed » Active

This can be considered now, but it looks like CiviCRM calls both these methods, so we might have to fully deprecate this somehow?

http://grep.xnddx.ru/search?text=setFieldNames&filename=

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 9.0.x-dev » 9.1.x-dev

Since 8.9.x and 9.0.x are now in beta, the way to do this would be to trigger a deprecation warning when the method is called from outside the class(es). We'd add that in a minor release of Drupal 9, and then that BC layer would be removed as part of the deprecation cleanup in Drupal 10. Thanks!

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.

bhanu951’s picture

bhanu951 commented

Seems this issue has passed 10.x release as well, we need to plan it for 11.x release.

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.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Just stumbled upon this. Currently there is not really an easy way to modify the default table mapping, in my opinion. Unless you actually want something radically different providing a different (sub-)class other than DefaultTableMapping, while possible, is not actually very helpful, because you may need to copy the entire class just to change a few lines. Having these methods public, currently allows avoiding that and actually making small alterations with just a small amount of code. See #3310170: Use UUID as entity ID for something that is currently fixable fairly cleanly from custom code which would become impossible, not in principle, but in reality, if these methods were made protected.

Since this also does not bring any maintainability gain as far as I can tell, I am against this until we have a different more bespoke API for actually customizing the entity table layout.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue tags: +DevDaysBurgas2024

arunkumark made their first commit to this issue’s fork.

arunkumark opened merge request !9228

arunkumark’s picture

arunkumark
Location Coimbatore
commented
Status: Active » Needs review
Related issues: +#3458145: Replace DefaultTableMapping::setFieldNames() with ::add*() and ::remove*()

The methods setFieldNames() and setExtraColumns() are called inside the class only. So, it can be converted into the protected.

There is another conversion of renaming setFieldNames the function on issue #3458145: Replace DefaultTableMapping::setFieldNames() with ::add*() and ::remove*(). If the approach is taken place we have to reroll this patch by making a protected method.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Re #16: As I noted in #12 these methods are helpful for contrib/custom code, so I don't think your argument is very convincing

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

For more discussion.

Also don't believe we can change this in a minor so if we still decide to update we can't do until D12

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.