Problem/Motivation

See #3566536: [meta] eliminate core .module files.

Way back in #1868772: Convert filters to plugins filters were converted to plugins. But these mostly consist of one line process methods.

  public function process($text, $langcode) {
    return new FilterProcessResult(_filter_autop($text));
  }

  public function process($text, $langcode) {
    return new FilterProcessResult(_filter_html_escape($text));
  }

Proposed resolution

Deprecate the following procedural functions and move their login in the corresponding filter plugin:

  • _filter_url()
  • _filter_url_parse_full_links()
  • _filter_url_parse_email_links()
  • _filter_url_parse_partial_links()
  • _filter_url_escape_comments()
  • _filter_url_trim()
  • _filter_autop()
  • _filter_html_escape()
  • _filter_html_image_secure_process()

No replacement is provided.

Remaining tasks

None.

User interface changes

None.

API changes

Above procedural code is deprecated.

Data model changes

None.

Issue fork drupal-3226806

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

longwave created an issue. See original summary.

longwave opened merge request !1014

longwave’s picture

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

Need to add deprecations etc if we are going to do this, will wait to see if anyone else thinks this is a good idea first. Doing this removes ~150 lines of procedural code from filter.module.

joachim’s picture

joachim commented

Looks like a good idea to me.

Functions that begin with an underscore were always considered as private/internal to Drupal, so I don't think we need to preserve these.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

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.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Functions that begin with an underscore were always considered as private/internal to Drupal, so I don't think we need to preserve these.

I think they should be deprecated, not removed. As they are publicly available, 3rd-party may using them.

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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Agree that we have to deprecate, several contrib.modules call _filter_autop at least, I didn't check the others but we should be consistent.

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

I guess the question now is do we move the functions pretty much as-is to static methods, so contrib can call them more easily?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: +Needs change record
joachim’s picture

joachim commented

> I guess the question now is do we move the functions pretty much as-is to static methods, so contrib can call them more easily?

They were supposed to be private functions! It was clearly documented in D5/6/7 days that functions starting with an underscore are private!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Discussed with @joachim in Slack, we decided that instead of exposing the filters as static methods any callers should instantiate the plugin they require via the plugin manager, e.g.

$text = \Drupal::service('plugin.manager.filter')
  ->createInstance('filter_autop')
  ->process($text, LanguageInterface::LANGCODE_NOT_SPECIFIED)
  ->getProcessedText();

We could perhaps shorten this to

$text = (string) \Drupal::service('plugin.manager.filter')
  ->createInstance('filter_autop')
  ->process($text);
joachim’s picture

joachim commented

Rebased.

Deprecations still to do.

The preg_replace() callbacks such as _filter_url_parse_full_links() should be deprecated too.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

MRs base branch should be changed to "main"

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.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Parent issue: » #3566536: [meta] eliminate core .module files

Adding to #3566536: [meta] eliminate core .module files

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: Unassigned » claudiu.cristea

Let's see

claudiu.cristea opened merge request !14530

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue tags: -Needs change record
  • The initial MR could not be resuscitated. I've opened a new one
  • Linked the existing generic CR

claudiu.cristea changed the visibility of the branch 3226806-move-filter-implementations to hidden.

joachim’s picture

joachim commented

Yup, I had a go at rebasing the old MR, and I hit a conflict which was caused by changes in the code the MR was deleting. IMO that's where resolving gets risky, because you're comparing two big chunks of code to check nothing was lost. Better to start from scratch.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes

Updated IS. This is ready for review.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: claudiu.cristea » Unassigned
joachim’s picture

joachim commented

I would have the deprecated functions call the plugin, so the code is only in one place.

joachim’s picture

joachim commented

Also, I thought it was a bad thing to inject the container directly into a class.

Would the better pattern not to get the parameter value and pass that to the constructor?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

#3558292: Support passing container parameters with the Autowire attribute in AutowireTrait and AutowiredInstanceTrait would make #27 cleaner.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

I would have the deprecated functions call the plugin, so the code is only in one place

The plugin expects also a language. I preferred not to touch. But, we might pass the default language

joachim’s picture

joachim commented

With the code in two places, it creates an extra maintenance burden until we remove the deprecated functions.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Title: Move filter implementations from filter.module to plugin classes » [PP-1] Move filter implementations from filter.module to plugin classes
Status: Needs work » Postponed

Also, I thought it was a bad thing to inject the container directly into a class.

OK, I prefer to block on #3558292: Support passing container parameters with the Autowire attribute in AutowireTrait and AutowiredInstanceTrait

I would have the deprecated functions call the plugin, so the code is only in one place.

With the code in two places, it creates an extra maintenance burden until we remove the deprecated functions.

Ok, ok... Let's do it when this is unpostponed

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Title: [PP-1] Move filter implementations from filter.module to plugin classes » Move filter implementations from filter.module to plugin classes
Assigned: Unassigned » claudiu.cristea
Status: Postponed » Needs work

The blocker was merged. I'm looking into this...

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: claudiu.cristea » Unassigned
Status: Needs work » Needs review

Addressed the remarks. Ready for review

nicxvan’s picture

nicxvan commented

I think this is almost ready.

I think these should be for removal in 12 since they are all underscore.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

I think this is ready!

I confirmed the deprecation versions are correct and match.

The functions call the correct plugins or retain the logic if appropriate.

This is more complex than a few others and a bit unique, but this seems like the way to do with with minimal disruption.

I also don't think breaking up the functions really makes this that much easier to review is four plugins basically impunity or adding methods.

nicxvan’s picture

nicxvan commented

Rebased since the other Filter issue just got in, it was clean so leaving RTBC

amateescu’s picture

amateescu commented
Status: Reviewed & tested by the community » Needs work

Reviewed :)

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

@amateescu, thank you for review. Addressed your remarks

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

All of @amateescu's comments have been addressed, the new split of escapeComments and unescapeComments actually makes this a lot clearer as well, my follow up comments have been addressed and the cleanups look good!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

Just one minor nit, fine to self RTBC afterwards.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Reviewed & tested by the community

Set as RTBC as per #42

amateescu’s picture

amateescu commented

We need a 11.x MR here, let's create it before the commit to main this time :)

nicxvan opened merge request !15407

nicxvan’s picture

nicxvan commented

Done!

  • amateescu committed 0347262d on 11.x 
    task: #3226806 Move filter implementations from filter.module to plugin...

  • amateescu committed ec8c0566 on main 
    task: #3226806 Move filter implementations from filter.module to plugin...
amateescu’s picture

amateescu commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed ec8c056 to main and 0347262 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.

amateescu closed merge request !14530

amateescu closed merge request !15407

Status: Fixed » Closed (fixed)

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