Problem/Motivation

FilterFormat file has @todo to rename the variable $format to $id and $name to $label to fit with other entities.

Proposed resolution

Do the renaming as said.

Remaining tasks

- rename
- RTBC the patch
- commit

User interface changes

None

API changes

None: those variables are private and should be accessed through getter/setters.

Data model changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is a small @todo to remove.
Issue priority Normal because it is only about code cleaning.
Unfrozen changes Unfrozen because it only changes internal variables name.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dom. created an issue. See original summary.

Dom.’s picture

Attaching patch, simply renamed $format to $id and $name to $label.

Status: Needs review » Needs work

The last submitted patch, 2: remove-todo-in-filterformat--2559377-2.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: remove-todo-in-filterformat--2559377-2.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: remove-todo-in-filterformat--2559377-2.patch, failed testing.

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
469 bytes
Xano’s picture

Title: Remove @todo in FilterFormat: rename $format and $name to $id and $name » Rename $format and $name to $id and $name in FilterFormat (per @todo)
Status: Needs review » Reviewed & tested by the community

Thanks! RTBC under condition that the tests pass.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2559377-8.patch, failed testing.

subhojit777’s picture

Title: Rename $format and $name to $id and $name in FilterFormat (per @todo) » Rename $format and $name to $id and $label in FilterFormat (per @todo)
subhojit777’s picture

Looks like there are many dependent tests based on this. I think we should break it down into separate issue and make this as meta issue. This way the patch will be easier to review.

joshi.rohit100’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.02 KB
899 bytes

Status: Needs review » Needs work

The last submitted patch, 13: 2559377-13.patch, failed testing.

Xano’s picture

I forgot to mention this earlier, but this change needs an upgrade path for sites that already have these configuration entities imported. During the update, use the configuration system to load the configuration (NOT the entity), change the keys, and save it again.

martins.kajins’s picture

martins.kajins’s picture

Assigned: Unassigned » martins.kajins
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: core-rename_format_and_name_to_id_and_label-8-16.patch, failed testing.

joshi.rohit100’s picture

Blank patch :)

joshi.rohit100’s picture

Blank patch :)

martins.kajins’s picture

Status: Needs work » Needs review
FileSize
595 bytes

Sorry for posting empty patch previously.

Status: Needs review » Needs work

The last submitted patch, 21: core-rename_format_and_name_to_id_and_label-8-21.patch, failed testing.

The last submitted patch, 21: core-rename_format_and_name_to_id_and_label-8-21.patch, failed testing.

martins.kajins’s picture

martins.kajins’s picture

Assigned: martins.kajins » Unassigned

this change needs an upgrade path for sites that already have these configuration entities imported. During the update, use the configuration system to load the configuration (NOT the entity), change the keys, and save it again.

singh_haneet’s picture

Attaching patch and interdiff.

joshi.rohit100’s picture

What I seems in #26, patch and diffs got interchanged. Patch should be diff and diff should be patch. Right ?

singh_haneet’s picture

No, the patch is only for "FilterFormat.php" file. Interdiff was created for #21 in which "filter.format.plain_text.yml" file was changed which has been reverted in this diff.

subhojit777’s picture

Status: Needs work » Needs review
subhojit777’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 26: 2559377-26.patch, failed testing.

The last submitted patch, 26: 2559377-26.patch, failed testing.

keopx’s picture

Assigned: Unassigned » keopx
keopx’s picture

Assigned: keopx » Unassigned

Status: Needs work » Needs review

keopx queued 26: 2559377-26.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 26: 2559377-26.patch, failed testing.

martins.kajins’s picture

Change $format to $id
change $name to $label

martins.kajins’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 37: core-rename_format_and_name_to_id_and_label-8-37.patch, failed testing.

Dom.’s picture

Is the patch #37 tested or failing ? I don't really understand the status of this issue...

martins.kajins’s picture

Change $format to $id

Status: Needs review » Needs work

The last submitted patch, 41: core-rename_format_and_name_to_id_and_label-8-40.patch, failed testing.

The last submitted patch, 37: core-rename_format_and_name_to_id_and_label-8-37.patch, failed testing.

The last submitted patch, 41: core-rename_format_and_name_to_id_and_label-8-40.patch, failed testing.

martins.kajins’s picture

Status: Needs review » Needs work

The last submitted patch, 45: core-rename_format_and_name_to_id_and_label-8-45.patch, failed testing.

The last submitted patch, 45: core-rename_format_and_name_to_id_and_label-8-45.patch, failed testing.

DuaelFr’s picture

Reading the last patch I think that something went wrong between #13 and now.
I think we should restart from #13 and find a way to fix the related tests without getting out of scope.

claudiu.cristea’s picture

Sorry, I don't think this is a good idea in this stage of releasing the RC. I would move it to 8.1.x.

DuaelFr’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

Agreed.
Thank you all for your work on this issue. Let's reopen it in a few weeks :)

jaxxed’s picture

I made these patches last week, but did not get a chance to upload them. I know that this issue is postponed, but I wanted to get the changes into the issue body.

Changes:
- id/label changes in tests (config, install, actual test calls);
- id/label change in the core standard install profile;
- id/label change in module install config;
- id/label change in module schema;
- reverted some incorrect changes of $format to $id, in cases where the $format was actually an entity object.

All of these changes are necessary in order to make the change as filter formats are create using the incorrect keys in all of those settings.

WARNING: don't approve these patches without testing them. There was a case where an exception was thrown related to having both an id key, and an 'edit-form' link. If it occurs during testing, then simply disable that link in the entity definition (annotation.)

DuaelFr’s picture

That's better! Thank you @jaxxed!
I had a quick look at your patch and it looks quite good.
Some comments follows, for the next person that's going to work on this.

  1. +++ b/core/modules/filter/filter.module
    @@ -171,7 +171,7 @@ function filter_get_roles_by_format(FilterFormatInterface $format) {
       foreach (filter_formats() as $format) {
    -    $roles = filter_get_roles_by_format($format);
    +    $roles = filter_get_roles_by_format($id);
    

    $id might be undefined according to the foreach().

  2. +++ b/core/modules/filter/src/Tests/FilterHooksTest.php
    @@ -41,22 +41,21 @@ function testFilterHooks() {
    +    $label = $this->randomMachineName();
    +    $format_id = Unicode::strtolower($this->randomMachineName());
    

    Inconsistent. Either use $format_label and $format_id, or $label and $id.

jaxxed’s picture

If someone looks at that last patch, this looks like a mistake:

index 46da1f0..7c611f9 100644
--- a/core/modules/filter/filter.module
+++ b/core/modules/filter/filter.module
@@ -171,7 +171,7 @@ function filter_get_roles_by_format(FilterFormatInterface $format) {
 function filter_get_formats_by_role($rid) {
   $formats = array();
   foreach (filter_formats() as $format) {
-    $roles = filter_get_roles_by_format($format);
+    $roles = filter_get_roles_by_format($id);
     if (isset($roles[$rid])) {
       $formats[$format->id()] = $format;
     }

EDIT: this is mentioned in the previous comment, tag you're it.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

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.

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.

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.

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.

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.