Follow-up to: #1779026: Convert Text Formats to Configuration System

Tasks

  1. Use ->id() instead of ->format everywhere.
  2. Use ->label() instead of ->name everywhere.
  3. Rename FilterFormat::$format to FilterFormat::$id.
  4. Rename FilterFormat::$name to FilterFormat::$label.
  5. Change entity router paths to contain /manage.
  6. Add FilterFormat::uri().
  7. Decipher, verify, and fix default weight behavior for new filters (docs-code-intention-logic mismatch).
  8. Do not save disabled filters whose properties are identical to all default properties.
  9. Replace obsolete _filter_list_cmp() with drupal_sort_title().
  10. Rename filter_html setting keys 'filter_html_help' and 'filter_html_nofollow' to 'show_help' and 'nofollow'.
  11. Rename $filter->title to $filter->label.
  12. Replace #type 'item' before filter status table with #title + #type 'table'.
  13. Replace #type 'item' before filter order table with #title + #type 'table'.
  14. Rethink filter sorting to remove dependency on filter IDs in FilterFormatStorageController.
  15. Automatically turn the first created (and only) text format into the fallback format.

Comments

tim.plunkett’s picture

sun’s picture

Status: Postponed » Active
sun’s picture

Issue summary: View changes

Updated issue summary.

heyrocker’s picture

Issue tags: +Configuration system
sun’s picture

Status: Active » Postponed

Postponed on #1868772: Convert filters to plugins, since that performs some massive changes to Filter module.

sun’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Status: Postponed » Active
Issue tags: +Novice, +php-novice

The blocker went in a long time ago.

Many of the subtasks are novice-level, so tagging as such.

sun’s picture

Assigned: sun » Unassigned

I most probably won't have time to work on this (but happy to review patches).

tim.plunkett’s picture

Some of this is covered by the linked issue.

drupaledmonk’s picture

Status: Active » Needs review
FileSize
1.81 KB
chx’s picture

chx’s picture

Status: Needs review » Reviewed & tested by the community

I have looked at this patch (because filter) and it doesn't have a security hole -- it's hard to imagine how it could have one -- otherwise it's also good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: cleanup-text-format-filter-module-1881664-8.patch, failed testing.

thechanceg’s picture

It looks like filter.module changed enough to prevent #8 from applying cleanly. Rerolled & tested locally.

dagmar’s picture

Status: Needs work » Needs review
k0teg’s picture

Issue tags: +Amsterdam2014

I'm here at DrupalCon Amsterdam and I'm going to review the patch.

k0teg’s picture

The patch #12 applies cleanly.

basvanderheijden’s picture

The patch did not apply cleanly for me, because the filter module has probably been changed between the initial posting by @thechangeg and the current state of the D8 beta.

To resolve, I've rerolled a new patch

k0teg’s picture

I have cleanly applied patch #16 and implemented subtasks 3 and 4.

basvanderheijden’s picture

I have created an interdiff and implemented subtasks 1 and 2.

webmorozov’s picture

The patch #18 applies cleanly.

The last submitted patch, 17: cleanup-filter-module-format-1881664-17.patch, failed testing.

basvanderheijden’s picture

I've reverted the additions made by @k0teg in #17: they crashed the whole filter format system.

I think we first have to look at references made elsewhere to the base properties FilterFormat::format and FilterFormat::name before being able to safely change the attributes. I'm also afraid #17 breaks the text module, which depends on the filter module.

The last submitted patch, 18: cleanup-filter-module-format-1881664-18.patch, failed testing.

basvanderheijden’s picture

Status: Needs review » Needs work

@sun: I'm not sure if the comments in the original issue summary still apply, because the issue is actually quite old. So here are some concerns I have. It would be great if you could have a look at those, so I can continue my work:

- The first two remarks were (easily) fixed in #21.
- About the third and fourth comment: aren't the id() and label() methods around to have consistency to the 'outside world'? To clarify: I've changed the code to fix the first two remarks, so outside of the class, the id() and label() methods are used instead of refering directly to the properties on the class (->name and ->format). But inside the class, isn't it ok (and even encouraged) to use more descriptive properties instead of the generalized ones? So isn't using ->format and ->name properties (and then having an id() and label() method returning those properties) ok?
- Could you update the seventh subtask? What still needs to be done here?
- About the 11th subtask: I couldn't find any references to $filter->title anymore. Deprecrated issue?
- Could you elaborate on 12 & 13 & 14?

Status: Needs work » Needs review

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.

Mile23’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll

Code changes for a task go to 8.2.x.

Patch doesn't apply.

chishah92’s picture

Assigned: Unassigned » chishah92
chishah92’s picture

I faced some conflicts while re-rolling , i fixed those conflicts and rerolled the patch and i found out that everything which was in the patch is already fixed in 8.2.x

chishah92’s picture

Assigned: chishah92 » Unassigned
Status: Closed (fixed) » Needs work

Sorry mistakenly changed the status. that comment for the previous patch only

vprocessor’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

rerolled

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.

ZeiP’s picture

Issue tags: -Needs reroll

Removing the reroll tag as that's been taken care of.

Mile23’s picture

Status: Needs review » Closed (works as designed)

The patch in #30 only adds a blank line.

As #28 points out, all the substantive changes seem to be in core already. So I'm marking this as closed (works as designed).