Opened 12 years ago
Last modified 4 months ago
#16502 reviewing enhancement
Quick Edit contents should only be rendered if quick edit button in actions after filtering
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | normal | Version: | 3.0.4 |
Component: | Quick/Bulk Edit | Keywords: | has-testing-info dev-feedback has-unit-tests 2nd-opinion |
Focuses: | Cc: |
Description (last modified by )
_the_post()
has a static call to get_inline_data()
to generate the quick_edit
content. This happens even if after running the (undocumented) post_row_actions
filters the Quick-Edit link ($actions['inline hide-if-no-js']
) is not amongst the actions anymore.
Prefixing the get_inline_data()
call with
if (isset($actions['inline hide-if-no-js'])
would be a simple solution, enabling people to cancel the performance-impacting quick-edit form generation alltogether.
Attachments (4)
Change History (54)
#2
@
12 years ago
- Milestone changed from Awaiting Review to Future Release
- Type changed from defect (bug) to enhancement
#6
@
14 months ago
- Description modified (diff)
- Keywords needs-testing added
- Milestone set to Future Release
Also, Core disables quick edit for (reusable) blocks.
Sample filter to use for testing other post types:
add_filter( 'post_row_actions', 'list_row_actions_remove_quick_edit', 10, 1 ); function list_row_actions_remove_quick_edit( $actions ) { unset( $actions['inline hide-if-no-js'] ); return $actions; }
This ticket was mentioned in Slack in #core by abhanonstopnews. View the logs.
11 months ago
#10
@
10 months ago
Hi there!
I have tested patch 16502.1.diff and it not working for me. After the patch when i click on Quick Edit
it will remove all the input values from the edit form.
@SergeyBiryukov @sabernhardt Is it working for you?
#11
@
10 months ago
- Keywords needs-patch added; has-patch needs-testing removed
Wow. I apparently did not test with post types where the Quick Edit button *is* available (unless maybe something changed since I tested). We need a new patch.
The column_title
function is not going to find $actions['inline hide-if-no-js']
when it does not make sure the $actions
array is set first.
Thanks for testing!
This ticket was mentioned in Slack in #core by marybaum. View the logs.
10 months ago
This ticket was mentioned in PR #3152 on WordPress/wordpress-develop by mukeshpanchal27.
9 months ago
#13
- Keywords has-patch added; needs-patch removed
Trac ticket:
#15
@
9 months ago
- Keywords needs-testing added
Ping @sabernhardt @SergeyBiryukov for the peer review so we can move forward on this ticket.
#16
@
9 months ago
- Keywords has-testing-info changes-requested added
Test Report
Patch tested: https://github.com/WordPress/wordpress-develop/pull/3152
Steps to Reproduce or Test
- Create a new file
wp-content/plugins/test-16502.php
with the following contents:<?php /** * Plugin Name: Test #16502 * Description: For testing <a href='https://core.trac.wordpress.org/ticket/16502'>16502</a>. * Author: WordPress Core Contributors * Author URI: https://make.wordpress.org/core * License: GPLv2 or later * Version: 1.0.0 */ add_action( 'init', 'test_16502_cpt' ); add_action( 'admin_bar_menu', 'test_16502_adminbar', 999 ); add_filter( 'post_row_actions', 'test_16502_hide_quick_edit', 10, 1 ); add_filter( 'page_row_actions', 'test_16502_hide_quick_edit', 10, 1 ); add_filter( 'tag_row_actions', 'test_16502_hide_quick_edit', 10, 1 ); add_filter( 'comment_row_actions', 'test_16502_hide_quick_edit_comments', 10, 1 ); function test_16502_cpt() { register_post_type( '16502', array( 'public' => true, 'label' => 'Test 16502' ) ); } function test_16502_adminbar( $wp_admin_bar ) { global $pagenow; $action = empty( $_GET['hide_quick_edit'] ) ? '1' : '0'; $url = add_query_arg( array_merge( $_GET, array( 'hide_quick_edit' => $action ) ), admin_url( $pagenow ) ); $wp_admin_bar->add_node( array( 'id' => 'test-16502', 'title' => $action ? 'Remove Quick Edit' : 'Add Quick Edit', 'href' => $url ) ); } function test_16502_hide_quick_edit( $actions ) { if ( isset( $_GET['hide_quick_edit'] ) && '1' === $_GET['hide_quick_edit'] ) unset( $actions['inline hide-if-no-js'] ); return $actions; } function test_16502_hide_quick_edit_comments( $actions ) { if ( isset( $_GET['hide_quick_edit'] ) && '1' === $_GET['hide_quick_edit'] ) unset( $actions['quickedit'] ); return $actions; }
- Navigate to
Plugins > Installed Plugins
and activate Test #16502. - Navigate to
Posts
. - Open DevTools and inspect a post's title.
- Below the post's title
<strong>
markup, there will be<div class="hidden" id="inline_XXXX">
element. - In the admin bar, click "Remove Quick Edit".
- 🐞 Repeat steps 4-5.
- Click "Add Quick Edit".
- Apply the PR.
- ✅ Repeat steps 3-7.
You may also test the following screens:
Posts > Categories
Posts > Tags
Pages
Test 16502
Expected Results
When reproducing a bug:
- ❌ The hidden "inline_XXXX" element should exist even though "Quick Edit" is removed.
When testing a patch to validate it works as expected:
- ✅ Posts (and other screens): The element should no longer exist when "Quick Edit" is removed.
Environment
- Server: Apache (Linux)
- WordPress: 6.1-alpha-53344-src
- Browser: Chrome 104.0.0.0
- OS: Windows 10
- Theme: Twenty Twenty-Two
- Plugins:
- Test #16502 1.0.0
Actual Results
When reproducing a bug/defect:
- ✅ Issue reproduced: The hidden "inline_XXXX" element still exists even though "Quick Edit" is removed.
When testing the bugfix patch:
- ✅ Posts: The hidden "inline_XXXX" element no longer exists when "Quick Edit" is removed.
- ✅ Pages: The hidden "inline_XXXX" element no longer exists when "Quick Edit" is removed.
- ✅ Custom Post Type (Test 15602): The hidden "inline_XXXX" element no longer exists when "Quick Edit" is removed.
- ❌ Tags: The hidden "inline_XXXX" element still exists even though "Quick Edit" is removed.
- ❌ Categories: The hidden "inline_XXXX" element still exists even though "Quick Edit" is removed.
Notes
- The patch works for Posts, Pages and Custom Posts Types, so this could be committed as-is.
- However, IMO, the patch should fix Tags and Categories too.
- Also, I didn't get to test with
Comments
, but I think the patch should cover this too.
#17
@
9 months ago
Thanks for the test report and feedback, @costdev.
As requested, I implemented the changes for the category and tags. I also tried to comment and got an error in the unit test for Ajax.
1) Tests_Ajax_EditComment::test_as_admin Undefined index: hook_suffix /var/www/src/wp-admin/includes/class-wp-screen.php:223 /var/www/src/wp-admin/includes/template.php:2601 /var/www/src/wp-admin/includes/class-wp-list-table.php:149 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:55 /var/www/src/wp-admin/includes/list-table.php:61 /var/www/src/wp-admin/includes/template.php:446 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:872 /var/www/src/wp-admin/includes/class-wp-list-table.php:1455 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:644 /var/www/src/wp-admin/includes/ajax-actions.php:1477 /var/www/src/wp-includes/class-wp-hook.php:307 /var/www/src/wp-includes/class-wp-hook.php:331 /var/www/src/wp-includes/plugin.php:517 /var/www/tests/phpunit/includes/testcase-ajax.php:265 /var/www/tests/phpunit/tests/ajax/EditComment.php:62 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51 2) Tests_Ajax_EditComment::test_editor_can_edit_orphan_comments Undefined index: hook_suffix /var/www/src/wp-admin/includes/class-wp-screen.php:223 /var/www/src/wp-admin/includes/template.php:2601 /var/www/src/wp-admin/includes/class-wp-list-table.php:149 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:55 /var/www/src/wp-admin/includes/list-table.php:61 /var/www/src/wp-admin/includes/template.php:446 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:872 /var/www/src/wp-admin/includes/class-wp-list-table.php:1455 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:644 /var/www/src/wp-admin/includes/ajax-actions.php:1477 /var/www/src/wp-includes/class-wp-hook.php:307 /var/www/src/wp-includes/class-wp-hook.php:331 /var/www/src/wp-includes/plugin.php:517 /var/www/tests/phpunit/includes/testcase-ajax.php:265 /var/www/tests/phpunit/tests/ajax/EditComment.php:110 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51 3) Tests_Ajax_GetComments::test_as_admin Undefined index: hook_suffix /var/www/src/wp-admin/includes/class-wp-screen.php:223 /var/www/src/wp-admin/includes/template.php:2601 /var/www/src/wp-admin/includes/class-wp-list-table.php:149 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:55 /var/www/src/wp-admin/includes/list-table.php:61 /var/www/src/wp-admin/includes/template.php:446 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:872 /var/www/src/wp-admin/includes/class-wp-list-table.php:1455 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:644 /var/www/src/wp-admin/includes/ajax-actions.php:1265 /var/www/src/wp-includes/class-wp-hook.php:307 /var/www/src/wp-includes/class-wp-hook.php:331 /var/www/src/wp-includes/plugin.php:517 /var/www/tests/phpunit/includes/testcase-ajax.php:265 /var/www/tests/phpunit/tests/ajax/GetComments.php:59 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51 4) Tests_Ajax_ReplytoComment::test_as_admin Undefined index: hook_suffix /var/www/src/wp-admin/includes/class-wp-screen.php:223 /var/www/src/wp-admin/includes/template.php:2601 /var/www/src/wp-admin/includes/class-wp-list-table.php:149 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:55 /var/www/src/wp-admin/includes/list-table.php:61 /var/www/src/wp-admin/includes/template.php:446 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:872 /var/www/src/wp-admin/includes/class-wp-list-table.php:1455 /var/www/src/wp-admin/includes/class-wp-comments-list-table.php:644 /var/www/src/wp-admin/includes/ajax-actions.php:1403 /var/www/src/wp-includes/class-wp-hook.php:307 /var/www/src/wp-includes/class-wp-hook.php:331 /var/www/src/wp-includes/plugin.php:517 /var/www/tests/phpunit/includes/testcase-ajax.php:265 /var/www/tests/phpunit/tests/ajax/ReplytoComment.php:73 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
I found a similar error report #29933 If I make these changes, it will resolve my unit test errors. @SergeyBiryukov, could you please help me with this?
/wp-admin/includes/class-wp-screen.php on line 223 $id = $GLOBALS['hook_suffix']; Replace with $id = isset($GLOBALS['hook_suffix']) ? $GLOBALS['hook_suffix'] : "";
peterwilsoncc commented on PR #3152:
9 months ago
#19
I've merged in trunk to resolve the merge conflict following a4ec93f438988e1df0e81e93a43e8d706ba22089
peterwilsoncc commented on PR #3152:
9 months ago
#20
Hmm, resolving merge conflict resulted in the ajax failing in PHP 8.1, so I need to hold off merging for now. I'm checking to see if I did something silly.
#21
@
9 months ago
- Keywords changes-requested removed
The PR got enough approval for the merge. Please review and share your thought.
mukeshpanchal27 commented on PR #3152:
9 months ago
#22
We are near to Beta 1.
As note by @costdev https://github.com/WordPress/wordpress-develop/pull/3152#discussion_r962906194 If we update the WP_Screen::get()
code https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/includes/class-wp-screen.php#L221-L225 then it works without adding the global $hook_suffix;
for the comment changes in 8.1
if ( $hook_name ) { $id = $hook_name; } else { $id = $GLOBALS['hook_suffix']; }
Replace with
if ( $hook_name ) { $id = $hook_name; } elseif ( wp_doing_ajax() && isset( $_REQUEST['action'] ) ) { $id = 'wp_ajax_' . esc_attr( $_REQUEST['action'] ); } elseif ( isset( $GLOBALS['hook_suffix'] ) ) { $id = $GLOBALS['hook_suffix']; } else { $id = ''; // Or just let it fail here. }
I'm happy to update it if it makes sense, or remove the quick edit changes for the comment and add a new issue in trac for it. Any thoughts, @peterwilsoncc @SergeyBiryukov?
Additionally, I checked some old tickets that have issues with hook_suffix
#38761 in which they update unit test code. Ping @johnbillion if you have any thoughts on this.
#23
@
9 months ago
- Milestone changed from 6.1 to Future Release
With 6.1 Beta 1 releasing tomorrow and this ticker still needing discussion and a decision, this is being moved to Future Release
.
If any maintainer/committer wishes to assume ownership of this during a specific cycle, feel free to update the milestone at that time.
#24
follow-up:
↓ 25
@
8 months ago
- Focuses performance removed
This ticket is not performance related so I am removing the tag.
#25
in reply to:
↑ 24
@
8 months ago
Replying to adamsilverstein:
This ticket is not performance related so I am removing the tag.
I believe the performance improvement mentioned in the ticket description was to avoid unnecessarily adding hidden fields with the Quick Edit data if they are not going to be used, though I agree the effect of that would likely be small, unless the post has a lot of terms, or something resource-heavy is attached to the add_inline_data
filter.
peterwilsoncc commented on PR #3152:
8 months ago
#26
@costdev @mukeshpanchal27 Are you able to refresh this now the hook fix has been merged?
8 months ago
#27
@mukeshpanchal27 Let me know whether you have time to refresh, or if you want me to handle it :slightly_smiling_face:
mukeshpanchal27 commented on PR #3152:
8 months ago
#28
@peterwilsoncc @costdev I have refreshed PR code. Can we set 6.1 milestone for this ticket?
8 months ago
#29
@mukeshpanchal27 that's an enhancement ticket so it'll have to wait for 6.2 at this point as all features/enhancements needed to be merged before 6.1 Beta 1.
mukeshpanchal27 commented on PR #3152:
8 months ago
#30
Thanks Jeff.
#31
@
8 months ago
- Milestone changed from Future Release to 6.2
Move to 6.2
consideration as PR is ready for final review and merge.
cc. @costdev @peterwilsoncc
This ticket was mentioned in Slack in #core by abhanonstopnews. View the logs.
6 months ago
This ticket was mentioned in Slack in #core by costdev. View the logs.
4 months ago
#34
@
4 months ago
Test Report
This report validates that the indicated patch addresses the issue (except Tags & Categories pages).
Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/3152.diff
Environment
- OS: macOS 12.6 (21G115)
- Web Server: Nginx
- PHP: 7.4.33
- WordPress: 6.2-alpha-54642-src
- Browser: Google Chrome
- Theme: Twenty Twenty-Three
- Active Plugins:
- Test #16502
Actual Results
- ✅ Issue resolved with patch (except Tags & Categories pages).
Additional Notes
- ❌ Patch doesn't fix for 'Categories' & 'Tags' pages.
Supplemental Artifacts
For All Posts page:
Before: https://prnt.sc/EfHw1uVHEv4V
After1: https://prnt.sc/beU3ORq-0OPt
After2: https://prnt.sc/s-TKp0dcCzKH
For Categories page:
Before: https://prnt.sc/g6UVGd5_KKE2
After: https://prnt.sc/u6BI8F2oCz86
For Tags page:
Before: https://d.pr/i/MppgtQ
After: https://prnt.sc/hf_zU66u9JBK
#35
@
4 months ago
Test Report
This report validates that the indicated patch addresses the issue (excluding Tags and Categories pages).
Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/3152.diff
Environment
- OS: macOS 11.2.3 (20D91)
- Web Server: Nginx
- PHP: 7.4.30
- WordPress: 6.2-alpha-54642-src
- Browser: Chrome 109.0.5414.119
- Theme: Twenty Twenty-Three
- Active Plugins:
- Test #16502
Actual Results
- ✅ Issue resolved with patch (excluding Tags and Categories).
Additional Notes
- ✅ All Posts Page: Issue resolved.
- ❌ Tags Page: Element still exists
- ❌ Categories Page: Element still exists
Supplemental Artifacts
All Posts:
Before - https://d.pr/i/y5u70O
After - https://d.pr/i/H5oaZQ
Tags Page:
Before - https://d.pr/i/YtmruC
After - https://d.pr/i/WD2VYA
Categories Page:
Before - https://d.pr/i/bDG1KJ
After - https://d.pr/i/BEEC3f
#36
@
4 months ago
@costdev @mukesh27 do you think we can commit without Tags and Categories pages?
Currently it fixes All posts page.
I am okay with/without Tags and Categories pages fix.
#37
@
4 months ago
Thanks @faguni22 @robinwpdeveloper for the test report.
I think for the category and tags we can add followup ticket so we get merge this ~12 years old ticket. WDYT @costdev?
#38
@
4 months ago
January 30, 2023 scrub:
- we have tested the patch and have the same results that the patch does not work for categories and tags
- can we determine whether a revised patch could fix categories and tags, or whether this can be committed on one aspect to get into 6.2, with a new ticket opening up for categories and tags fix.
@costdev do you have a view please?
#39
@
4 months ago
Hi all. Yes, the categories and tags will need some more consideration as there's possibly a little bit of refactoring to do to enable us to prevent the "inline_XXXX"
div from being output.
Until then, I think we can proceed with PR 3152 for 6.2 and continue the work for Categories and Tags in 6.3.
If committers agree, we can create the follow-up ticket once the PR has been committed and the ticket resolved as fixed
.
Pinging @peterwilsoncc and @SergeyBiryukov for their thoughts as two committers who have reviewed the PR.
#40
@
4 months ago
- Keywords needs-dev-note added
@costdev Splitting the patches up makes sense but if it's across milestone, let's set up a second ticket to avoid moving tickets around once they have commits on them.
My once concern with this change is if a plugin author has modified the actions but relies on the div being present for some custom code. Adding needs-dev-note accordingly.
#41
@
4 months ago
- Keywords needs-testing removed
That sounds good to me Peter!
As this ticket has multiple test reports that show that all non-Category/Tag functionality is working, I'll remove needs-testing
.
#43
@
4 months ago
Hi! I wonder if I am missing something, another patch possibly.
I looked up and tested the patch PR 3152 with WP v 6.1.1 and:
$actions['inline hide-if-no-js']
is set, so, inline data is still added.- Reply form in comments is moved across, and the result is that hidden data is still present, but forms are not working properly for the last item in comments list (please, look the video above).
- Browser is not requesting data to edit by ajax at the moment of click on the Quick edit link and just filling in the form with existing data which can be obsolete at the time of editing if, for example, several editors are fixing things at the same time. So, I think that the whole point of not rendering editing form is to be sure that the data is actual at the moment of editing.
- Browser will not render invisible content until it become visible, so, improvement in performance when unnecessary data will be removed is quite small.
Please, consider the purpose of the ticket and take a second look for what tables and how are affected.
#44
@
4 months ago
- Keywords dev-feedback added
Thanks @oglekler! I believe you've spotted a valid issue with the patch. It seems that the Quick Edit
and Reply
features both use markup generated by the wp_comment_reply()
function, and that the patch is missing consideration of the reply
action.
Adding isset( $actions['reply'] ) ||
to the start of the conditions at src/wp-admin/includes/class-wp-comments-list-table.php:867
appears to resolve the issue.
However, since this particular markup is not isolated to one feature (covers both Quick Edit
and Reply
), we might need to consider whether changing the Comments screen is necessary here.
For now, I'm thinking that we should remove the changes to src/wp-admin/includes/class-wp-comments-list-table.php
and investigate the Comments screen in a separate ticket.
Given that we'll be addressing Tags/Categories/Other Terms in #57596, I think the PR should probably just target src/wp-admin/includes/class-wp-posts-list-table.php
to remove unnecessary rendering in Posts/Pages/Custom Post Type list tables. After all, the ticket description only refers to post_row_actions
/get_inline_data()
.
@oglekler @mukesh27 @peterwilsoncc What do you think?
#45
@
4 months ago
- Keywords has-unit-tests added
I've attached some PHPUnit tests to cover a final patch.
These cover:
- A post with the
inline hide-if-no-js
action. - A page with the
inline hide-if-no-js
action. - A hierarchical custom post type with the
inline hide-if-no-js
action. - A non-hierarchical custom post type with the
inline hide-if-no-js
action. - A post without the
inline hide-if-no-js
action. - A page without the
inline hide-if-no-js
action. - A hierarchical custom post type without the
inline hide-if-no-js
action. - A non-hierarchical custom post type without the
inline hide-if-no-js
action.
#46
@
4 months ago
- Keywords commit added
PR 3152 has been updated to only contain changes for Posts/Pages/Custom Post Types and add the tests above so that we can address Tags/Categories and Comments in a later cycle.
This looks ready for commit
consideration to me.
Additional props: @mukesh27
This ticket was mentioned in Slack in #core by abhanonstopnews. View the logs.
4 months ago
#48
@
4 months ago
Does this result in a change to the position of elements within the DOM? It looks like the output is still within the same td cell but it would be good to confirm.
#49
@
4 months ago
- Milestone changed from 6.2 to 6.3
This ticket was discussed in the pre-6.2 Beta 1 bug scrub. Moving to 6.3 so that a committer can weigh in.
#50
@
4 months ago
- Keywords 2nd-opinion added; has-patch needs-dev-note commit removed
Having some doubts about this patch. It effectively disables one of the WordPress features: Quick Edit. As far as I know this feature is not used very often, but is quite helpful for Editors on sites with many users/authors.
The disabling is done indirectly, by "piggybacking" on other/existing methods. Don't think this is a good way to disable a feature. A better way would be to have a filter where extenders will be able to disable it directly.
In addition the current code may introduce bugs/regressions in plugins that replace the "Quick Edit" action link with another method of showing the UI, or reuse the quick edit data in some way.
Ooops, second closing bracket is missing above.