Prevent assigning error to `$this->usage` by kienstra · Pull Request #40 · cloudinary/cloudinary_wordpress

kienstra · 2020-01-30T17:28:00Z

There was a fatal error reported on activation:

An error of type E_ERROR was caused in line 340 of the file /path/to/
wp-content/plugins/cloudinary_wordpress_v2/php/sync/class-push-sync.php. Error
message: Uncaught Error: Cannot use object of type WP_Error as array in
/path/to/wp-content/plugins/cloudinary_wordpress_v2/php/sync/
class-push-sync.php:340

cloudinary_wordpress/cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/sync/class-push-sync.php

Line 340 in 9b10a98

if ( $file_size > $this->plugin->components['connect']->usage[ $max_size ] ) {

I couldn't reproduce that, with the same versions as reported in the bug:
PHP 7.4
WP Core 5.3.2
Foodoholic theme

But this PR addresses one possible cause of the error.

It might be possible that $this->plugin->components['connect']->usage is a WP_Error, causing this fatal error.

It looks like the code anticipates $stats being a WP_Error:

cloudinary_wordpress/cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/class-connect.php

Line 285 in 9b10a98

if ( ! is_wp_error( $stats ) && ! empty( $stats['media_limits'] ) ) {

If so, it will be assigned to $this->plugin->components['connect']->usage:

cloudinary_wordpress/cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/class-connect.php

Line 291 in 9b10a98

$this->usage = $stats;

...possibly causing the error the user reported.

Still, I can't reproduce the error that was reported. So it's only a guess that this might fix it.

This could have been a cause of the error on activating the plugin, though I couldn't reproduct it, even with the same PHP and WP versions and theme. But it looks like the code anticipates $stats possibly being an error. In that case, it probably shouldn't be assigned to $this->usage.

kienstra · 2020-01-30T17:45:13Z

Request For Review

Hi @dugajean,
Thanks a lot for reviewing #39. Could you please review this?

Thanks, Dugi!

This should still not allow this to be stored.

This can still be set as $this->usage, it should just exit.

kienstra · 2020-01-30T18:03:43Z

Sorry, not planning on making any more commits before review 😄

parabrola · 2020-01-30T18:32:45Z

cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/class-connect.php

+
+				if ( is_wp_error( $stats ) ) {
+					return;
+				}
+
+				if ( ! empty( $stats['media_limits'] ) ) {


I think it might be a better idea to set some defaults before returning if there's a WP_Error since there are 5 uses of its keys around the plugin, which might cause undefined index errors.

See class-media.php at line 1079-1080

See class-push-sync.php at line 340-341

See class-upload-sync.php at line 120

What do you think @kienstra ?

Uf, good point

kienstra · 2020-01-30T19:05:48Z

Closing

This PR is only a guess at one way the fatal error reported could have happened. Since I can't reproduce the issue, I don't think it makes sense to continue.

If there really was a WP_Error from calling the API:

cloudinary_wordpress/cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/connect/class-api.php

Line 581 in 9b10a98

    
           return new \WP_Error( $request['response']['code'], $result['error']['message'] );

...then there would need to be better UI handling for this. The user would probably need to know that there was some kind of error in order to try again.

Simply setting 'max_image_size' to a default like 0 or PHP_INT_MAX doesn't seem fair, as it might not have been their fault.

I don't think we can 'fix' something that's not reproducible.

const-cloudinary · 2020-02-02T09:38:54Z

@kienstra , I can reproduce this, we have an environment that fails with this issue.

kienstra · 2020-02-03T03:39:30Z

@const-cloudinary,
Thanks, are there any other active plugins, other than Cloudinary? And what theme is active?

The stack trace here unfortunately doesn't point to where the WP_Error could be coming from.

const-cloudinary · 2020-02-03T09:54:37Z

@kienstra, I would start with invalid CLOUDINARY_URL, when "usage" api returns 40x/50x response or something like that.

const-cloudinary · 2020-02-03T14:12:31Z

@kienstra I printed the error. it says:
disabled account in /home/consta01/public_html/wp-content/plugins/cloudinary_wordpress_v2 5/php/sync/class-push-sync.php...

kienstra · 2020-02-03T20:42:11Z

Hi @const-cloudinary,
Thanks for your help.

@kienstra I printed the error. it says:
disabled account in /home/consta01/public_html/wp-content/plugins/cloudinary_wordpress_v2 5/php/sync/class-push-sync.php...

Would you be able to paste the rest of the error, especially the line number?

const-cloudinary · 2020-02-04T10:38:44Z

@kienstra It's the same line of the "Cannot use object of type WP_Error as array" . WP_Error contains this message.

You can reproduce it by deleting your Cloudinary account, clicking "Cancel my account" on this page:
https://cloudinary.com/console/settings/account

kienstra · 2020-02-06T07:40:44Z

Hi @const-cloudinary,
Thanks for helping with the step to reproduce this.

There didn't seem to be a PHP error after canceling my Cloudinary account:

I also tried wp transient delete _cloudinary_usage, and then activating the plugin.

const-cloudinary · 2020-02-06T17:25:39Z

@kienstra Can you please provide me some code that I can run on my environment (add some logging entries/write notice message on the screen, etc), so I can understand what is exactly happening in my environment?

kienstra · 2020-02-06T17:37:08Z

Hi @const-cloudinary,
Thanks, do you have xdebug enabled? That's probably going to be easier than var_dump() or print() statements.

Also, this debugging tool has helped me a lot: https://github.com/nette/tracy

const-cloudinary · 2020-02-09T13:39:13Z

@kienstra This bug is reproducible on a shared hosting website I own, it will be problematic to install xdebug there.

kienstra · 2020-02-09T18:06:30Z

Hi @const-cloudinary,
OK, thanks.

Is that a production site? If not, would it be possible to deploy this PR to it, to see if it fixes it?

If it's a production site, would it be possible to create a staging 'clone' of it?

Thanks!

kienstra requested a review from parabrola January 30, 2020 17:45

kienstra added 3 commits January 30, 2020 11:51

Restore check for is_wp_error()

28bceed

This should still not allow this to be stored.

Exit if the API call returned a WP_Error

a8e69f9

This can still be set as $this->usage, it should just exit.

Remove needless empty line

8a555c2

parabrola reviewed Jan 30, 2020

View reviewed changes

kienstra closed this Jan 30, 2020

pereirinha deleted the update/address-fatal-error-activation branch July 22, 2020 11:10

Comments

Conversation

kienstra commented Jan 30, 2020

Uh oh!

kienstra commented Jan 30, 2020

Uh oh!

kienstra commented Jan 30, 2020

Uh oh!

parabrola Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

kienstra Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

kienstra commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

const-cloudinary commented Feb 2, 2020

Uh oh!

kienstra commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

const-cloudinary commented Feb 3, 2020

Uh oh!

const-cloudinary commented Feb 3, 2020

Uh oh!

kienstra commented Feb 3, 2020

Uh oh!

const-cloudinary commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kienstra commented Feb 6, 2020

Uh oh!

const-cloudinary commented Feb 6, 2020

Uh oh!

kienstra commented Feb 6, 2020

Uh oh!

const-cloudinary commented Feb 9, 2020

Uh oh!

kienstra commented Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kienstra commented Jan 30, 2020 •

edited

Loading

kienstra commented Feb 3, 2020 •

edited

Loading

const-cloudinary commented Feb 4, 2020 •

edited

Loading

kienstra commented Feb 9, 2020 •

edited

Loading