Quick Edit contents should only be rendered if quick edit button in actions after filtering

[wyrfel]

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

[wyrfel]

Ooops, second closing bracket is missing above.

[garyc40]

simple patch

[sabernhardt]

adding curly brackets

[sabernhardt]

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;
}

[mukesh27]

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?

[sabernhardt]

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!

Trac ticket:

[mukesh27]

I have submitted PR that fix the issue can you please review it?

[mukesh27]

Ping @sabernhardt @SergeyBiryukov for the peer review so we can move forward on this ticket.

[costdev]

Test Report

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/3152

Steps to Reproduce or Test

1. 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;
   }
   

2. Navigate to Plugins > Installed Plugins and activate Test #16502.
3. Navigate to Posts.
4. Open DevTools and inspect a post's title.
5. Below the post's title <strong> markup, there will be <div class="hidden" id="inline_XXXX"> element.
6. In the admin bar, click "Remove Quick Edit".
7. 🐞 Repeat steps 4-5.
8. Click "Add Quick Edit".
9. Apply the PR.
10. ✅ 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.

[mukesh27]

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'] : "";

[mukesh27]

Thanks @costdev for help to solve the issue. PR is ready for review.

I've merged in trunk to resolve the merge conflict following a4ec93f438988e1df0e81e93a43e8d706ba22089

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.

[mukesh27]

The PR got enough approval for the merge. Please review and share your thought.

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.

[davidbaumwald]

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.

[adamsilverstein]

This ticket is not performance related so I am removing the tag.

[SergeyBiryukov]

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.

@costdev @mukeshpanchal27 Are you able to refresh this now the hook fix has been merged?

@mukeshpanchal27 Let me know whether you have time to refresh, or if you want me to handle it :slightly_smiling_face:

@peterwilsoncc @costdev I have refreshed PR code. Can we set 6.1 milestone for this ticket?

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

Thanks Jeff.

[mukesh27]

Move to 6.2 consideration as PR is ready for final review and merge.

cc. @costdev @peterwilsoncc

[faguni22]

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

[robinwpdeveloper]

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

[robinwpdeveloper]

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

[mukesh27]

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?

[webcommsat]

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?

[costdev]

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.

[peterwilsoncc]

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

[costdev]

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.

[costdev]

I've created #57596 to address the Categories/Tags part of this.

@mukesh27 Are there any parts of ​PR 3152 that should be removed and addressed in #57596 instead, before we move ​PR 3152 forward for commit consideration?

[oglekler]

Comments table

[oglekler]

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.

[costdev]

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?

[costdev]

PHPUnit tests.

[costdev]

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.

[costdev]

​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

[johnbillion]

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.

[costdev]

This ticket was discussed in the pre-6.2 Beta 1 bug scrub. Moving to 6.3 so that a committer can weigh in.

[azaozz]

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.