Problem/Motivation

There are only four remaining functions in media.module
Let's move them and deprecate if necessary.
_media_library_views_form_media_library_after_build
_media_library_media_type_form_submit
_media_library_configure_form_display
_media_library_configure_view_display

Steps to reproduce

N/A

Proposed resolution

Move to hook class and mark internal. They need to be public because they are form callbacks.

  • _media_library_views_form_media_library_after_build
  • _media_library_media_type_form_submit

Move to helper class that is internal. Make static so install can use them as well.

  • _media_library_configure_form_display
  • _media_library_configure_view_display

Remaining tasks

Review
Got signoff from a subsystem maintainer

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3570839

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

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan opened merge request !14578

nicxvan’s picture

nicxvan commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs work

Looks straightforward, just a couple of questions. I approve as a subsystem maintainer.

nicxvan’s picture

nicxvan commented

We resolved those questions!
Needs work due to an unrelated failure on HEAD: #3570848: AssetAggregationAcrossPagesTest::testNodeAddPagesAuthor fails locally on main

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

This is ready for review now!

nicxvan’s picture

nicxvan commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Fine with me.

nicxvan’s picture

nicxvan commented

Just noting here we might be getting new direction on underscore functions to just delete them outright like we usually do.

Once I confirm I'll update this if necessary.

nicxvan’s picture

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

got new direction to just delete underscore functions per the policy.

nicxvan’s picture

nicxvan commented
Title: Deprecate remaining functions in media_library.module » Delete remaining underscore functions in media_library.module
dcam’s picture

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

All declarations and usages of the underscore functions have been removed from Core.

nicxvan’s picture

nicxvan commented
Title: Delete remaining underscore functions in media_library.module » Deprecate remaining underscore functions in media_library.module
Status: Reviewed & tested by the community » Needs review

Sorry for the ping pong - we got confirmation that callbacks and underscore can be deprecated for removal in 12.
Just waiting on tests.

dcam’s picture

dcam commented
Status: Needs review » Needs work

One of the docblocks didn't get edited. I left a suggestion.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
dcam’s picture

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

The deprecated functions have been restored. There are no usages of these functions remaining in Core, only their declarations. All of the restored functions have proper deprecations. LGTM.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs rebase.

nicxvan opened merge request !14952

nicxvan closed merge request !14952

nicxvan’s picture

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

I rebased, I think it's fine to self rtbc.

longwave’s picture

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

Think we're going to need separate MRs here as this now applies to main but not 11.x.

nicxvan opened merge request !14953

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Done, not sure I can self rtbc this, but they are identical.

nicxvan’s picture

nicxvan commented

I can't look at the tests on 11.x but I'd be surprised if they were not random.

nicxvan’s picture

nicxvan commented

Since we have split MRs now I asked in slack if we should just outright remove the functions on main, @longwave confirmed we should so I did.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Gotta fix phpstan.

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Rebased both MRs through the UI, the 11.x was 190 commits behind but there were no conflicts. Back to RTBC.

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

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

This is close, but let's try strict types on the new class for both MRs.

nicxvan’s picture

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

I think it's safe to self RTBC.

godotislate closed merge request !14953

godotislate closed merge request !14578

  • godotislate committed 9ea78bec on main 
    refactor: #3570839 Deprecate remaining underscore functions in...

  • godotislate committed bc6fedfe on 11.x 
    refactor: #3570839 Deprecate remaining underscore functions in...
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Fixed

Committed 9ea78be and pushed to main.
Committed bc6fedf and pushed 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.