Make WordPress Core

Opened 13 years ago

Closed 9 months ago

Last modified 9 months ago

#16502 closed enhancement (fixed)

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

Reported by: wyrfel's profile wyrfel Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.4 Priority: normal
Severity: normal Version: 3.0.4
Component: Quick/Bulk Edit Keywords: has-patch needs-testing commit
Focuses: Cc:

Description (last modified by sabernhardt)

_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 (5)

16502.diff (652 bytes) - added by garyc40 13 years ago.
simple patch
16502.1.diff (441 bytes) - added by sabernhardt 2 years ago.
adding curly brackets
core-ticket-16502.mp4 (3.4 MB) - added by oglekler 17 months ago.
Comments table
16502-tests.diff (4.4 KB) - added by costdev 17 months ago.
PHPUnit tests.
16502.2.diff (3.9 KB) - added by SergeyBiryukov 10 months ago.

Change History (69)

#1 @wyrfel
13 years ago

Ooops, second closing bracket is missing above.

#2 @nacin
13 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

@garyc40
13 years ago

simple patch

#3 @SergeyBiryukov
13 years ago

  • Keywords has-patch added; quick edit filters removed

#4 @chriscct7
10 years ago

  • Severity changed from minor to normal

#5 @chriscct7
9 years ago

  • Owner set to chriscct7
  • Status changed from new to reviewing

@sabernhardt
2 years ago

adding curly brackets

#6 @sabernhardt
2 years 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;
}

#7 @sabernhardt
2 years ago

  • Milestone changed from Future Release to 6.1

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


2 years ago

#9 @SergeyBiryukov
2 years ago

  • Focuses performance added

#10 @mukesh27
22 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 @sabernhardt
22 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.


22 months ago

This ticket was mentioned in PR #3152 on WordPress/wordpress-develop by mukeshpanchal27.


22 months ago
#13

  • Keywords has-patch added; needs-patch removed

Trac ticket:

#14 @mukesh27
22 months ago

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

#15 @mukesh27
22 months ago

  • Keywords needs-testing added

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

#16 @costdev
22 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

  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:

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.
Last edited 17 months ago by costdev (previous) (diff)

#17 @mukesh27
22 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'] : "";

#18 @mukesh27
22 months ago

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

peterwilsoncc commented on PR #3152:


22 months ago
#19

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

peterwilsoncc commented on PR #3152:


22 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 @mukesh27
21 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:


21 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 @davidbaumwald
21 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: @adamsilverstein
21 months ago

  • Focuses performance removed

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

#25 in reply to: ↑ 24 @SergeyBiryukov
21 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:


21 months ago
#26

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

costdev commented on PR #3152:


21 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:


21 months ago
#28

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

jeffpaul commented on PR #3152:


20 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:


20 months ago
#30

Thanks Jeff.

#31 @mukesh27
20 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.


19 months ago

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


17 months ago

#34 @faguni22
17 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:

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 @robinwpdeveloper
17 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:

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 @robinwpdeveloper
17 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 @mukesh27
17 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 @webcommsat
17 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 @costdev
17 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 @peterwilsoncc
17 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 @costdev
17 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.

#42 @costdev
17 months ago

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
17 months ago

Comments table

#43 @oglekler
17 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 @costdev
17 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?

Last edited 17 months ago by costdev (previous) (diff)

@costdev
17 months ago

PHPUnit tests.

#45 @costdev
17 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 @costdev
16 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.


16 months ago

#48 @johnbillion
16 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 @costdev
16 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 follow-up: @azaozz
16 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.

#51 @oglekler
12 months ago

  • Milestone changed from 6.3 to 6.4

I have doubts as well (#comment:43) we need clear understanding what need to be done and then compare it with patch. If we are improving performance, the result needs to be measured.

This is an enhancement, and we are in 12 days until Beta 1 after which we will not add new enhancement to the release, so, because there is no patch, I am moving this ticket to 6.4.

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


10 months ago

#53 @oglekler
10 months ago

  • Keywords changes-requested added; has-testing-info has-unit-tests removed
  • Milestone changed from 6.4 to Future Release

Because there is no movement lately, I am moving this ticket into Future release. If there will be a new patch, and it will be working correctly, it can go to the next available release.

#54 in reply to: ↑ 50 ; follow-up: @SergeyBiryukov
10 months ago

  • Keywords has-patch added; dev-feedback changes-requested removed
  • Milestone changed from Future Release to 6.4

Replying to 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.

Indeed, removing $actions['inline hide-if-no-js'] via the post_row_actions filter seems like a hacky way to disable the Quick Edit functionality.

Looking at the current PR, moving the get_inline_data() call to the handle_row_actions() method does not seem ideal either, because the former displays the markup directly, while the latter is supposed to return the result as a string and should not display anything.

16502.2.diff introduces two new filters to address both this ticket and #57596:

  • quick_edit_enabled_for_post_type
  • quick_edit_enabled_for_taxonomy

That seems like a cleaner way to do this, and should also resolve the backward compatibility concerns.

#55 @oglekler
10 months ago

  • Keywords needs-testing added

#56 @hellofromTonya
9 months ago

Hey @azaozz,

What do you think about @SergeyBiryukov approach in 16502.2.diff? Does it address your concerns?

#57 in reply to: ↑ 54 @azaozz
9 months ago

  • Keywords commit added; 2nd-opinion removed

Replying to SergeyBiryukov:

That seems like a cleaner way to do this, and should also resolve the backward compatibility concerns.

Yep, the new filters make sense imho.

Does it address your concerns?

Sure, LGTM now.

#58 follow-up: @peterwilsoncc
9 months ago

Naming things nitpick -- quick_edit_enabled_for_posts or quick_edit_enabled_for_post_objects may be better.

add_filter( 'quick_edit_enabled_for_post_type', '__return_false' ) disables the feature for all posts so could be seen as misleading.

Given @SergeyBiryukov, @azaozz, @hellofromTonya and @oglekler are all very experienced contributes, it's also possible I'm over thinking this so I won't be upset if you disregard this comment :)

#59 in reply to: ↑ 58 ; follow-up: @SergeyBiryukov
9 months ago

Replying to peterwilsoncc:

add_filter( 'quick_edit_enabled_for_post_type', '__return_false' ) disables the feature for all posts so could be seen as misleading.

I was aiming for consistency with the existing use_block_editor_for_post_type filter :)

Curious to see what others think.

#60 in reply to: ↑ 59 @peterwilsoncc
9 months ago

Replying to SergeyBiryukov:

I was aiming for consistency with the existing use_block_editor_for_post_type filter :)

That works for me.

#61 @SergeyBiryukov
9 months ago

  • Owner changed from chriscct7 to SergeyBiryukov
  • Status changed from reviewing to accepted

#62 @SergeyBiryukov
9 months ago

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

In 56611:

Quick Edit: Allow Quick Edit to be disabled for custom post types or taxonomies.

Some custom post types or taxonomies may not need the Quick Edit functionality, in which case adding hidden fields and rendering the form with the data to edit would be redundant.

This commit introduces two filters for more granular control:

  • quick_edit_enabled_for_post_type
  • quick_edit_enabled_for_taxonomy

Follow-up to [8857], [9083], [9098].

Props garyc40, sabernhardt, mukesh27, costdev, oglekler, wyrfel, peterwilsoncc, faguni22, robinwpdeveloper, webcommsat, johnbillion, azaozz, hellofromTonya, GunGeekATX, Jick, mikeschinkel, jane, nacin, helen, wonderboymusic, DrewAPicture, SergeyBiryukov.
Fixes #16502, #19343, #57596.

@SergeyBiryukov commented on PR #3152:


9 months ago
#63

Thanks for the PR! An alternative approach was merged in r56611.

#64 @mukesh27
9 months ago

Thank you @SergeyBiryukov for the great work.

Note: See TracTickets for help on using tickets.