woocommerce / action-scheduler

A scalable, traceable job queue for background processing large queues of tasks in WordPress. Specifically designed for distribution in WordPress plugins (and themes) - no server access required.
https://actionscheduler.org
GNU General Public License v3.0
641 stars 117 forks source link

Admin issues with 2.0.0 #151

Closed sirtawast closed 5 years ago

sirtawast commented 6 years ago

Hi,

thanks for all the great work to get action-scheduler to where it is now. It's really the most comprehensive solution for WordPress background worker, event/action queue I've tried.

There's a couple of major bugs in the latest beta release that prevents me to use it in production (I've been using a master commit build previously with better luck, thinking it was 591f66436).

My docker builds: WordPress / PHP-FPM wordpress:4.9.4-php7.1-fpm utilizing PHP 7.1.14

NGINX nginx:1.13.8


Delete posts

/wp-admin/tools.php?page=action-scheduler

Deletion of scheduled-action posts gives a blank admin template with following warning:

Warning: Cannot modify header information - headers already sent by (output started at /var/www/html/wp-admin/includes/template.php:2039) in /var/www/html/wp-includes/pluggable.php on line 1216

The deletion itself goes through.


Ordering posts by any column

/wp-admin/tools.php?page=action-scheduler

Changing the order of scheduled-action posts by any column gives the following error:

Fatal error: Uncaught TypeError: Argument 3 passed to ActionScheduler_ActionFactory::get_stored_action() must be of the type array, null given, called in /var/www/html/wp-content/plugins/action-scheduler/classes/ActionScheduler_wpPostStore.php on line 101 and defined in /var/www/html/wp-content/plugins/action-scheduler/classes/ActionScheduler_ActionFactory.php:17 

Stack trace: 
#0 /var/www/html/wp-content/plugins/action-scheduler/classes/ActionScheduler_wpPostStore.php(101): ActionScheduler_ActionFactory->get_stored_action('pending', 'my_scheduled_task...', NULL, Object(ActionScheduler_SimpleSchedule), 'my_bg_request') 
#1 /var/www/html/wp-content/plugins/action-scheduler/classes/ActionScheduler_wpPostStore.php(77): ActionScheduler_wpPostStore->make_action_from_post(Object(WP_Post)) 
#2 /var/www/html/wp-content/plugins/action-scheduler/classes/ActionScheduler_ListTable.php(486): ActionScheduler_wpPostStore->fetch_action('41359') 
#3 /var/www/html/wp-content/plugins/action-scheduler/vendor/pp-list-table/src/class-pp-list-table.php( in /var/www/html/wp-content/plugins/action-scheduler/classes/ActionScheduler_ActionFactory.php on line 17

If you need further data to debug, I'm happy to provide that to you.

JPry commented 6 years ago

Thanks for all of the information @sirtawast. I’ll be looking into this today.

sirtawast commented 6 years ago

The order uses wrong column keys, at least date is working with params &orderby=date&order=asc. That's what the old build had.

Same thing with status, works with &orderby=post_status&order=asc

JPry commented 6 years ago

@sirtawast I did some testing today with a local site and the latest version of Action Scheduler. I wasn't able to duplicate the issues you described. Would you be able to provide more information about the data that the actions contain?

Specifically with the second error, it looks like there may be bad data stored in the post_content field of the action. Could you shed some more light on how these actions were created?

Thanks!

sirtawast commented 6 years ago

@JPry thanks for trying out.

I wasn't able to duplicate the issues you described.

This is what I kind of expected because my issues are so visible 😬 Let's start digging!

I'll make sure to fiddle with a clean WP install and report back to you when I have the time, probably next week. Cheers!

JPry commented 6 years ago

@sirtawast No worries. We definitely welcome any bug reports you have so that we can ensure this library is bug-free.

I'm going to submit a PR today that includes less possibility for triggering the fatal error you described, even if we don't expect to encounter that situation very often.

sirtawast commented 6 years ago

Here's a testbench where you can repeat both issue on a clean install.

https://github.com/sirtawast/wp-testbench/tree/action-scheduler

PS: I've got the same problem with WordPress' official container as well. They are a PITA to hook up with NGINX though, so I went with modified wodby.

JPry commented 6 years ago

Hi @sirtawast,

Thanks for linking to that repository. However, I can't run that as-is due conflicts with Laravel Valet. It's also not clear to me how you're going about creating actions that are having a problem. Could you please provide steps to create actions that are having the problems you describe?

sirtawast commented 6 years ago

Hi @JPry,

Actually there's no actions that have to be registered at all. If I just sort the empty list, the errors will pop up. I will try to set you up with another env later on if this one is no good for you.

JPry commented 6 years ago

I've done some testing with the master branch of Action Scheduler and an empty Scheduled Actions list table. The master branch is currently the same as the 2.0.0-beta-2 tag. Here's what I've found:

Sort by Error
Hook none
Status none
Group [14-Jun-2018 14:11:06 UTC] WordPress database error Unknown column 't.name' in 'order clause' for query SELECT p.ID FROM wp_posts p WHERE post_type='scheduled-action' ORDER BY t.name DESC LIMIT 0, 10 made by require('wp-admin/tools.php'), require_once('wp-admin/admin.php'), do_action('tools_page_action-scheduler'), WP_Hook->do_action, WP_Hook->apply_filters, ActionScheduler_AdminView->render_admin_ui, PP_List_Table->display_page, ActionScheduler_ListTable->prepare_items, ActionScheduler_wpPostStore->query_actions
Scheduled Date none

After completing these tests, I purchased a subscription, which in turn created a Scheduled action:

Hook Status Arguments Group Recurrence Scheduled Date
woocommerce_scheduled_subscription_payment Pending subscription_id => 2785 Non-repeating 2019-06-14 14:15:15 +00:00

I repeated all of the sorting tests, and found that the results were the same: Sorting by group triggered an error.

I then used the bulk delete option to delete this action, and I did not get any error message.

From these tests, It looks like we need to fix the group sorting, especially when there isn't any group available. But I'm not able to reproduce the other problems you mentioned @sirtawast. If you have specific steps on how to reproduce those other items, I'll be glad to try again.

jonathanstegall commented 6 years ago

Just want to confirm that I also have this issue.

I'm not running WooCommerce, and I'm seeing this on a local environment (PHP 7.1, Nginx, Laravel Valet). I have define( 'WP_DEBUG_LOG', true ); in wp-config.

When I try to delete multiple records:

Warning: Cannot modify header information - headers already sent by (output started at /wp-includes/class.wp-styles.php:225) in /wp-includes/pluggable.php on line 1219

This happens every time I try to check the boxes for multiple rows and delete them. If I try to reload the page on the same screen, without navigating elsewhere, I get this error:

Fatal error: Uncaught InvalidArgumentException: Unidentified action 162136 in (plugin path)/vendor/prospress/action-scheduler/classes/ActionScheduler_wpPostStore.php:407 Stack trace: #0 (plugin path)/vendor/prospress/action-scheduler/classes/ActionScheduler_ListTable.php(404): ActionScheduler_wpPostStore->delete_action('162136') #1 (plugin path)/vendor/prospress/action-scheduler/classes/ActionScheduler_Abstract_ListTable.php(151): ActionScheduler_ListTable->bulk_delete(Array, '('162136','1621...') #2 (plugin path)/vendor/prospress/action-scheduler/classes/ActionScheduler_ListTable.php(458): ActionScheduler_Abstract_ListTable->process_bulk_action() #3 (plugin path) in (plugin path)/vendor/prospress/action-scheduler/classes/ActionScheduler_wpPostStore.php on line 407

If I click Scheduled Actions in the menu, instead of reloading, the page loads correctly and it has deleted the selected records.

I noticed this does not happen when I sort the table (by Hook, for example) but it does happen when I search for anything in the search box.

sirtawast commented 6 years ago

@jonathanstegall thanks for chipping in, seems to be related. I too have debug on. @JPry I still haven't had the time to set up an environment to reproduce. Sorry about that!

sirtawast commented 6 years ago

I just tested this last night on a clean WordPress 4.9.8 install with the newest 2.0.0. release. This time I was on Windows 10 and using Bitnami WAMP stack running Apache. The only same thing share with the dockerized version is PHP 7.1

jamesryder commented 5 years ago

I'll add them I'm seeing the same issues as @jonathanstegall in 2.1.1.

Using vvv (vagrant) with PHP 7.2.12 and WP 4.9.8, nginx 1.15.6. I'm also seeing the issue on my host.

rrennick commented 5 years ago

Can anyone provide a list of plugins that you have active?

jamesryder commented 5 years ago

I’ve removed all other plugins and switched to 2015 theme to test and have the same issues. I’ve been running a fairly lean install so I’ll test a clean install tomorrow.

Should add to test without any other plugins I’ve been adding actions using a simple loop in functions.php.

jamesryder commented 5 years ago

Clean install using vvv (vagrant) with: PHP 7.2.12 WP 4.9.8 nginx 1.15.6

Added actions using: as_schedule_single_action( time(), 'test_action' );

`Warning: Cannot modify header information - headers already sent by (output started at /srv/www/scheduler/public_html/wp-includes/formatting.php:5077) in /srv/www/scheduler/public_html/wp-includes/pluggable.php on line 1219

Same issue if I add a group to the action.

sirtawast commented 5 years ago

@rrennick

Can anyone provide a list of plugins that you have active?

@jamesryder

I’ve removed all other plugins and switched to 2015 theme to test and have the same issues. I’ve been running a fairly lean install so I’ll test a clean install tomorrow.

No plugins activated at all for me as well. Tested with stock installation for a clean MAMP, also with the official WordPress Docker stack.

Edit: Same kind of dummy function like James had.

rrennick commented 5 years ago

Clean install using vvv (vagrant) with: PHP 7.2.12 WP 4.9.8 nginx 1.15.6

Added actions using: as_schedule_single_action( time(), 'test_action' );

Where/what hook are you calling that in? I have Valet vs VVV but otherwise the same setup. I don't have any debug warnings when I call that inside of init.

jamesryder commented 5 years ago

Sorry for the delay @rrennick, to test I just created a shortcode that looped through and create 10 actions using as_schedule_single_action( time(), 'test_action' );. The test_action does nothing.

I have also also created actions using a plugin and got similar results.

rrennick commented 5 years ago

Here's the code I used to test AS 2.0.0 (WC), 2.1.0 (WCS), & master:

function test_action() {}

add_shortcode( 'testas', function() {
    for ( $i = 1; $i <= 10; $i++ ) {
    as_schedule_single_action( time(), 'test_action' );
    }
});

I didn't get a debug warning on any version.

I just created a shortcode

Shortcode functions are called well after headers have been output so you should never see an output already started message with a shortcode.

jamesryder commented 5 years ago

I only see the error when deleting the actions in Tools > Scheduled Actions either using Cancel or the drop down Delete. The delete completes but the warning is always displayed.

@rrennick I guess you're not seeing the error on delete?

rrennick commented 5 years ago

If there is an issue with deleting actions, I wouldn't expect that the method used to create them would matter. Since the discussion was focused on creating the actions, I hadn't looked at deleting them. I'm going to be another day or 2 before I have a chance to get back to this.

alanef commented 5 years ago

FYI I am glad in way you are seeing this error, as I am seeing it too in my docker system. I haven't had chance to investigate further - but as a noob to Action Scheduler initially thought it might be a conflict with what I was developing. If I get chance to narrow it down I will report back.

richard-stovall commented 5 years ago

Any update on this? I'm seeing the same issue using 2.2.5

richard-stovall commented 5 years ago

Looks like the issue stems from ActionScheduler_Abstract_ListTable.php:155. There is a wp_redirect() on that line, but headers were already sent before that method is processed. I'm able to resolve the issue by creating a custom redirect method and using that instead.

public function redirect($url = false)
    {
        if (headers_sent()) {
            $destination = ($url == false ? 'location.reload();' : 'window.location.href="' . $url . '";');
            echo die('<script>' . $destination . '</script>');
        } else {
            $destination = ($url == false ? $_SERVER['REQUEST_URI'] : $url);
            header('Location: ' . $destination);
            die();
        }
    }

And change the wp_redirect(...) to $this->redirect(...)

alanef commented 5 years ago

I have written classes that extend WP_List_Table and the process bulk section doesn't have a redirect,I don't think it is required, but haven't got time right now to test or investigate. Will look at it too if I get a chance next week.

rrennick commented 5 years ago

There is a wp_redirect() on that line, but headers were already sent before that method is processed.

Prior output is what causes Warning: Cannot modify header information. What we were trying to find in this issue was the source of the prior output.

alanef commented 5 years ago

Well that is a bit easy, it is the whole of the dashboard is output sometime before the process_bulk_action();

I think the code is trying to build up the bulk action then redirects to itself to execute the bulk action, I don't really understand why it is architecture is that way redirecting when it could just call methods.

None of the WP code seems to use redirects within list tables. Maybe this worked once upon a time, but certainly on any of my tests does not.

rrennick commented 5 years ago

None of the WP code seems to use redirects within list tables.

It does use redirects with list tables. The core WP dashboard load PHP files directly. Each of those files loads WP before handling requests, POSTs, etc. Because plugins rely on hooks, some code is going to be located differently than WP core.

alanef commented 5 years ago

I only had a quick look at plugins and posts list table code, and couldn't see redirects being used, doesn't mean there are not somewhere deeper, but that is a bit of a red herring and doesn't help sort out why the process actions of this code line is firing consistently after the admin page is rendered. I have had a poke around the code with xdebug but I'm not familiar enough wit the structure to provide any real help.

I suppose the javascript redirect is a workable work around, but that really doesn't feel the best solution by far.

alanef commented 5 years ago

Looks like the issue stems from ActionScheduler_Abstract_ListTable.php:155. There is a wp_redirect() on that line, but headers were already sent before that method is processed. I'm able to resolve the issue by creating a custom redirect method and using that instead.

public function redirect($url = false)
    {
        if (headers_sent()) {
            $destination = ($url == false ? 'location.reload();' : 'window.location.href="' . $url . '";');
            echo die('<script>' . $destination . '</script>');
        } else {
            $destination = ($url == false ? $_SERVER['REQUEST_URI'] : $url);
            header('Location: ' . $destination);
            die();
        }
    }

And change the wp_redirect(...) to $this->redirect(...)

Note also need to change the ActionScheduler_listTable.php wp_redirect ( line 461 )

This fix ( whilst hacky ) works perfectly for me.

johnbillion commented 5 years ago

I've just run into this redirect bug locally. I don't see how it would ever work because the prepare_items() method on list tables always runs after the admin toolbar, admin menu, header, etc has been output.

It seems that this redirect isn't necessary anyway. I'd vote to remove it in favour of using WordPress core's removable_query_args filter.

tillkruss commented 5 years ago

What’s the status on this. It was opened a year ago. Can we do a quick patch to avoid this redirect issues?

alessandrotesoro commented 5 years ago

I've just run into this issue too. Weirdly enough, if I activate WooCommerce, the issue disappears 👀

alanef commented 5 years ago

I found exactly the same,I know there are some specific woocommerce hooks, i tried to work out why it works with WC but not without, but failed.

thenbrent commented 5 years ago

if I activate WooCommerce, the issue disappears

That's a really good clue. Thanks for sharing @alessandrotesoro!

alanef commented 5 years ago

In case you missed it, I also gave the same 'really good clue' back on June 15 https://github.com/Prospress/action-scheduler/issues/315 :)

thenbrent commented 5 years ago

@alanef I did see that, but that I didn't realise it was the same issue highlights the issue with having two issues open for what I think is the same root cause, as described more concisely on #315. I'm going to close this one in favour of #315 now. Thanks for pointing it out!