Make WordPress Core

Opened 15 months ago

Closed 10 months ago

Last modified 10 months ago

#57612 closed defect (bug) (fixed)

Deprecate `wp-admin/media.php`

Reported by: kebbet's profile kebbet Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Media Keywords: early has-patch commit add-to-field-guide
Focuses: administration Cc:

Description

Is it time to deprecate the old Edit Media screen? I think so.

Reasons for removing it from core.

  • The file is not linked to from core.
  • The Edit image button (while editing images) leads to a visually broken screen.
  • The Insert into Post button leads to a white screen.
  • The Update Media button makes you leave the screen, taking you back to the media library list.

The file was introduced in #6181/[7262]. Not sure when it was removed from media workflow.
See #57608 for reference. Props to @costdev for also investigating this.

Attachments (1)

Screen Shot 2023-04-12 at 8.40.18 am.png (92.2 KB) - added by peterwilsoncc 12 months ago.

Download all attachments as: .zip

Change History (41)

#1 @kebbet
15 months ago

The screen is accessible via wp-admin/media.php?attachment_id=NNN&action=edit, where NNN is attachment id.

#2 @costdev
15 months ago

Here are some references to usage by plugins and themes. There is only one plugin with installs that has been tested up to the latest three major releases, or within the last two years.

If there are concerns about backward compatibility that mean this file should not be deprecated, that's understandable.

Otherwise, this file has been patched many times over the past number of years adding to the maintenance burden, and we should consider deprecating it.

#3 in reply to: ↑ description @SergeyBiryukov
15 months ago

Replying to kebbet:

The file was introduced in #6181/[7262]. Not sure when it was removed from media workflow.

I think it was removed in [21948] / #21391, see the change to _edit_link for the attachment post type:

-        '_edit_link' => 'media.php?attachment_id=%d', /* internal use only. don't use this when registering your own post type. */
+        '_edit_link' => 'post.php?post=%d', /* internal use only. don't use this when registering your own post type.

This ticket was mentioned in PR #3979 on WordPress/wordpress-develop by @kebbet.


15 months ago
#4

  • Keywords has-patch added; needs-patch removed

#5 @jrf
14 months ago

  • Focuses coding-standards removed

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


13 months ago

#7 @antpb
13 months ago

  • Keywords early added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 6.3

This ticket was discussed in a recent Media meeting and it was agreed that this should move forward and deprecate the view. A couple things noted.

@joedolson mentioned "If there are any functions here that need to be kept, they should be moved to wp-includes, anyway."

Also it was discussed that after this is removed we put in place a redirect for this pattern to go to the attachment page of the intended media.

Adding 6.3 and early so we can try to get this in as soon as possible to determine the impact.

@audrasjb commented on PR #3979:


13 months ago
#8

I refreshed the PR to update the since mention add for trunk merge purpose :)

@kebbet commented on PR #3979:


12 months ago
#9

I refreshed the PR to update the since mention add for trunk merge purpose :)

Should the last line also be updated to use 6.3.0 ?

@audrasjb commented on PR #3979:


12 months ago
#10

Ah yes, updated :)

#11 @kebbet
12 months ago

6.3 early is now, any chance the linked PR can be committed anytime soon?

#12 @audrasjb
12 months ago

  • Keywords commit added
  • Owner set to audrasjb
  • Status changed from new to accepted

Self assigning for a last review before commit.

#13 @audrasjb
12 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 55647:

Media: Deprecate wp-admin/media.php.

The wp-admin/media.php file was introduced in [7262], then removed from the Media workflow in [21948].
This changeset finally deprecates it as it is not used anymore.

Follow-up to [7262], [21948].

Props kebbet, costdev, SergeyBiryukov, jrf, antpb, audrasjb.
Fixes #57612.
See #6181, #21391, #57608.

#15 @peterwilsoncc
12 months ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

As seen in the screen shot above, visiting media.php following [55647] results in a white screen without any user facing information. As a number of plugins reference the file, the most popular with 200K+ installs, I don't think this is adequate.

I think a redirect to admin_url( '/uploads.php' ) is more suitable. If placed after the deprecation notice, a headers_sent() check will be required.

#16 @jorbin
12 months ago

I think we should redirect this page rather than having it white screen. I would suggest going to wp-admin/upload.php ideally with a notice that the previous page was removed in 6.3

apparently, @peterwilsoncc had the exact same thought.

#17 @audrasjb
12 months ago

You got it, and wp-admin/upload.php looks like the best redirect. Having a notice displayed after the redirection whould indeed be ideal.

I can't see any precedent for such a redirection notice. Would a notice like this do the job?

It looks like the screen you are looking for does not exist. You have been redirected to this page instead.

(has to be rephrased, just putting words together)

And with a redirection using a query var to display the notice?

As I couldn't find any precedent, just wanted to make sure we're doing it right :)

Last edited 12 months ago by audrasjb (previous) (diff)

#18 @azaozz
12 months ago

It could be an automatic redirect + a note, or perhaps a notice on wp-admin/media.php with a link to upload.php, i.e. info + manual redirect.

Seems both cases would need require_once __DIR__ . '/admin.php'; as it seems media.php was an entry point.

#19 @jorbin
12 months ago

And with a redirection using a query var to display the notice?

Yes. I would do it like that.

Riffing on your wording, The Edit Media screen is deprecated as of WordPress 6.3. Please use the Media Library instead

also, _deprecated_file causes a 500 error since the function isn't defined if you go directly to wp-admin/media.php. Seems to also be the case for wp-admin/custom-header.php

#20 @peterwilsoncc
12 months ago

Completely untested but how about something like the following code.

Unfortunately, upload.php removes the wp_admin_canonical_url action from the admin_init screen (not sure why) so the error would remain in the URL. It may be possible to reinstate it and use the removable_query_args filter instead. -- This may be a terrible idea, too :)

<?php
/**
 * Media management action handler.
 *
 * @package WordPress
 * @subpackage Administration
 */

/** Load WordPress Administration Bootstrap */
require_once __DIR__ . '/admin.php';

$parent_file  = 'upload.php';
$submenu_file = 'upload.php';

wp_reset_vars( array( 'action' ) );

switch ( $action ) {
        case 'editattachment':
        case 'edit':
                // Used in the HTML title tag.
                $title = __( 'Edit Media' );

                if ( empty( $errors ) ) {
                        $errors = null;
                }

                if ( empty( $_GET['attachment_id'] ) ) {
                        wp_redirect( admin_url( 'upload.php' ) );
                        exit;
                }
                $att_id = (int) $_GET['attachment_id'];

                wp_redirect( admin_url( "upload.php?item={$att_id}&error=throw-depreciated-media.php" ) );
                exit;

        default:
                wp_redirect( admin_url( 'upload.php?error=throw-depreciated-media.php' ) );
                exit;
}

#21 @NekoJonez
12 months ago

Just to understand, will we be unable to e.g. crop/rename attachments after this or will these features be moved to another place?

Since thats what is unclear to me now. What features is this going to remove (regardless if they are buggy or not) and what replacements do we have/are needed?

#22 follow-up: @kebbet
12 months ago

@NekoJonez Renaming, croping, editing etc. was moved to post.php in [21948], so no functionality is removed. What was removed in [55647] was the predecessor to the current workflow, which has beed broken for years, and is not accessible from any link in core.

@peterwilsoncc regarding the references to media.php in the 200000 installs plugin, the reference is only present in the *.pot-file of the plugin, no actual links. Same goes for all plugins above 5000 I looked at in the linked list (.po/.pot). The Media tags plugin with 4000 is closed.

This ticket was mentioned in PR #4320 on WordPress/wordpress-develop by @kebbet.


12 months ago
#23

  • Keywords has-patch added; needs-patch removed

https://core.trac.wordpress.org/ticket/57612

A follow up to https://core.trac.wordpress.org/changeset/55647.

Based on suggestions from @peterwilsoncc and @aaronjorbin

#24 in reply to: ↑ 22 @NekoJonez
12 months ago

Replying to kebbet:

@NekoJonez Renaming, croping, editing etc. was moved to post.php in [21948], so no functionality is removed. What was removed in [55647] was the predecessor to the current workflow, which has beed broken for years, and is not accessible from any link in core.

@peterwilsoncc regarding the references to media.php in the 200000 installs plugin, the reference is only present in the *.pot-file of the plugin, no actual links. Same goes for all plugins above 5000 I looked at in the linked list (.po/.pot). The Media tags plugin with 4000 is closed.

Thanks for the answer. Now, I'm going to put on my other hat as a polyglot.
Shouldn't these pot files be recreated then? So, translations still happen correctly?

#25 @azaozz
12 months ago

  • Keywords changes-requested added

#26 @kebbet
12 months ago

  • Keywords changes-requested removed

Linked PR has been updated. Please have another look at it.

@kebbet commented on PR #4320:


12 months ago
#27

{{{diff
return array( 'error' );
}}}

Whats the idea behind it?

@kebbet commented on PR #4320:


12 months ago
#28

{{{diff
return array( 'error' );
}}}

Whats the idea behind it?

@kebbet commented on PR #4320:


12 months ago
#29

What's the idea behind the suggested addition to wp-admin/uploads.php?

@peterwilsoncc commented on PR #4320:


12 months ago
#30

@kebbet The WordPress dashboard uses the JavaScript history API to remove querystring parameters that used to display one time messages/errors, it prevents the message showing if someone bookmarks the URL or copies and pastes it.

To see an example, visit /wp-admin/edit.php?trashed=1&ids=1 and you'll see the trashed message displayed but the querystring parameters will be removed via JavaScript.

As the error this PR is adding to the uploads screen is intended as a one time message, it would be good to make use of the feature to remove the error parameter.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


11 months ago

@kebbet commented on PR #4320:


11 months ago
#32

Thanks Colin for a review, adjusted the PR to align with your suggestion!

@kebbet commented on PR #4320:


11 months ago
#33

{{{diff
+ add_filter(
+ 'removable_query_args',
+ function( $query_args ) {
+ return array( 'error' );
+ }
+ );
}}}

@peterwilsoncc If I add this change to the file, there is an error in the editor, Symbol '$query_args' is declared but not used. Is that expected?

@peterwilsoncc commented on PR #4320:


11 months ago
#34

there is an error in the editor, Symbol '$query_args' is declared but not used. Is that expected?

That's expected and a false report as the code replaces the value entirely.

@kebbet commented on PR #4320:


11 months ago
#35

Ok, thanks for quick reply @peterwilsoncc !

I've updated the PR with the suggestion, with an addition of a one line comment, please review. And test.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


10 months ago

#37 @peterwilsoncc
10 months ago

  • Keywords commit added

The linked pull request looks good for a follow-up commit.

#38 @peterwilsoncc
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 55943:

Media: Redirect deprecated wp-admin/media.php file.

Redirect users visiting the wp-admin/media.php file to the media library, wp-admin/upload.php. An user facing warning is displayed when the media library is reached via a deprecated link.

Follow up to [55647].

Props jorbin, audrasjb, azaozz, NekoJonez, kebbet, costdev.
Fixes #57612.

#40 @stevenlinx
10 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.