Problem/Motivation

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

Usage in core & contrib: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=editor_i...

Proposed resolution

Deprecate and move to a trait EDIT: It was decided to move the logic to a new service.

Remaining tasks

None.

User interface changes

None.

Introduced terminology

None.

API changes

editor_image_upload_settings_form() is deprecated. Its logic is move to a service.

Data model changes

None.

Issue fork drupal-3570917

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

claudiu.cristea created an issue. See original summary.

claudiu.cristea opened merge request !14592

claudiu.cristea’s picture

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

Ready for review

dcam’s picture

dcam commented
Status: Needs review » Needs work

The existing changes look good, but there are some additional @see editor_image_upload_settings_form() references in Core that I assume should be updated. These were found with grep -r "editor_image_upload_settings_form" core.

nicxvan’s picture

nicxvan commented

Does this need to be a trait or can we just move the code into the form where it's used?

That test is really weird that it calls the function too.

claudiu.cristea’s picture

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

@nicxvan,

Does this need to be a trait or can we just move the code into the form where it's used?

The original function description states:

 * Each text editor plugin that is configured to offer the ability to insert
 * images, should use this form to update the text editor's configuration so
 * that it knows whether it should allow the user to upload images.

My understanding is that we have here a reusable piece of code which is supposed to be used not only by CKEditor5 but it's also offered to any 3rd-party provided editor.

@dcam,

Replaced the rest of occurrences

smustgrave’s picture

smustgrave commented

Pure some of the other ones should this be a service instead so services can be injected

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thinking maybe this needs to be a service like what was done on #3572339: Modernize locale.fetch.inc and #3568387: Move text_summary to TextSummary service and deprecate

nicxvan’s picture

nicxvan commented

I agree it should be a service since it itself uses services and based on discussions in slack adding services to traits is very complex and not at all solved.

The unfortunate thing here is that it's only really used by one thing, but it's also duplicated in a test.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Humbly suggesting a better search link: https://search.tresbien.tech/search?q=editor_image_upload_settings_form&...

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
Status: Needs work » Needs review
  • Converted the trait to a service
  • Updated IS & CR
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Title: Deprecate editor_image_upload_settings_form() and move the logic to a trait » Deprecate editor_image_upload_settings_form() and move the logic to a service
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Ready for review

nicxvan’s picture

nicxvan commented

This looks much better!

One trivial comment on the file deprecation notice.

I think this is nearly ready.

nicxvan’s picture

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

Looks good I think this is ready!

godotislate closed merge request !14592

  • godotislate committed 4a57d7b3 on main 
    refactor: #3570917 Deprecate editor_image_upload_settings_form() and...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 4a57d7b and pushed to main. Thanks!

Did not apply to 11.x, so need a separate MR for that.

claudiu.cristea opened merge request !15096

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Patch (to be ported) » Needs review

Opened MR 15096 which is green. Please review

@godotislate, as a side note: the main was merged but the BC layer of deprecation of the constructor addition, with removal in Drupal 12 was not removed before merge. See https://git.drupalcode.org/project/drupal/-/commit/4a57d7b3ce085ad59f747.... Not sure exactly what is the rule, but some core committers are either removing themselves or ask for separate MRs. Should we open an issue to remove the BC layer in main?

godotislate’s picture

godotislate
he/him
commented

Should we open an issue to remove the BC layer in main?

That's what we did for #3555534: Since symfony/validator 7.4: Support for evaluating options in the base Constraint class is deprecated. Initialize properties in the constructor instead. and #3569092: Update method signatures for addConstraint() and ConstraintManager::create() and remove HasNamedArguments use, but let me see if we need consensus on process for this.

smustgrave’s picture

smustgrave commented

Will say I’ve been doing a lot of the deprecation removal for D12. Have most of it done minus a handful of tickets. I have a final ticket to get anything missed or new stuff so if it’s simple I’m happy to roll into that.

nicxvan’s picture

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

The backport is good, I think we just get as many of the .module deprecations in as we can then we do one big follow up to clean up the ones we got in.

  • larowlan committed c0336941 on 11.x 
    feat: #3570917 Deprecate editor_image_upload_settings_form() and move...

larowlan closed merge request !15096

larowlan’s picture

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

Backported 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.

larowlan’s picture

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

Looks like I committed the wrong thing here

  • longwave committed 87b75b4d on 11.x 
    refactor: #3570917 Deprecate editor_image_upload_settings_form() and...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 87b75b4da77 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.