search

humanmade / asset-manager-framework

A framework for overriding the WordPress media library with an external asset provider, such as a DAM
GNU General Public License v2.0
155 stars 7 forks source link

Fix fatal when media details or sizes are stdClass object #43

Closed PatelUtkarsh closed 3 years ago

PatelUtkarsh commented 3 years ago

Happens in the case of the video attachment.

See https://github.com/WordPress/wordpress-develop/blob/19751bd0d37000f910446d010d7ac9df6eb75d2d/src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php#L758

roborourke commented 3 years ago

Thanks for the PR! This avoids the issue but I think we still need to apply the fix in the event that it's a stdClass type. We could cast it to an array or do a json_encode() and json_decode( $data, ARRAY_A ) to ensure the data is what we expect for processing.

roborourke commented 3 years ago

If you're able to comment too I would love to know how you're using this framework and what upcoming features you see being the most useful.

PatelUtkarsh commented 3 years ago

@roborourke Thank you for the plugin and comment!

stdClass is set only when media details are empty or sizes are empty. See ref:

  1. https://github.com/WordPress/wordpress-develop/blob/19751bd0d37000f910446d010d7ac9df6eb75d2d/src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php#L756-L758
  2. https://github.com/WordPress/wordpress-develop/blob/19751bd0d37000f910446d010d7ac9df6eb75d2d/src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php#L789

Converting to array: Is it expecting other plugins may have it as a stdClass array with data? On a 2nd thought, there is no downside to converting it.

We are using the framework to integrate proprietary DAM integration with WordPress for a specific use case. In this use case, the DAM is the source of truth and has a good amount of assets to not bring all of them in WordPress so this plugin fits perfectly as the editor can "download" assets as required.

One of the use-cases is to delete the attachment from WordPress after a certain amount of time by using cron as we may delete the image from DAM if there is a legal kill request for it, If you would like to support that use-case in this plugin I will be happy to send PR for it.

A better way to handle the above use-case would be if DAM can notify WordPress when there is a delete request for the resource but this may be too specific.

roborourke commented 3 years ago

Ah sorry I missed that, makes sense with PHP data structures not properly matching JSON structures when empty arrays are converted.

I'm not sure how we'd handle sync from the upstream source yet in a standardised way but being able to delete an upstream resource is on the roadmap. The cron deletion is something you'll need to handle yourselves for now. Thanks for the extra info though, definitely to something to consider in future!