-
Notifications
You must be signed in to change notification settings - Fork 0
REFACTOR: Update codebase following PSR4 standards #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
chore: refactor to TS
| Attachment::update_is_syncing( $media['id'], 'sync' === $sync_option ); | ||
|
|
||
| // Share the attachment metadata with the brand sites. | ||
| // Get attachment metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're not going to validate the id or handle the false, we should at least coerce the false ?: []
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also below (~L1392)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are ensuring the value of media['id'] above here $media['id'] = isset( $media['id'] ) ? intval( $media['id'] ) : 0;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which part of that ensures that's a valid attachment ID and the db isnt locked, and whatever else that can cause wp_get_attachment_metadata() to return false? 🤨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please pay more attention to the feedback I give you.
- General rule: don't call a function when you can avoid it. Like what do you even gain conceptually by calling
wp_get_attachment_metadata( 0 )? - What you get IRL is the bug I was telling you to fix in the first place, which is that there are multiple ways for the function to return
falseinstead of an empty array (passing an invalid post id is just one of them: https://github.com/WordPress/wordpress-develop/blob/cd5b7b140a230713c06d97d2fd4a3dac947e0e5c/src/wp-includes/post.php#L6897), so therefore you should$attachment_data = wp_get_attachment_metadata( $media['id'] ) ?: [];
If this is still not clear, please DM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added ?? 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah thats the entire problem....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, somehow this comment is visible now. Added changes.
Co-authored-by: Dovid Levine <[email protected]>
| * | ||
| * @return array The array of sync site URLs. | ||
| */ | ||
| private static function get_sync_site_urls_postmeta( int $attachment_id ): array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have been moved below. ::health_check_attachment_brand_sites
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies. Remember the hierarchy, updated
Overview
This PR refactors the OneMedia plugin to follow PSR-4 autoloading standards and modern PHP best practices, improving code organization, maintainability, and developer experience.
Code Organization & Architecture
PSR-4 Autoloading: Restructured all PHP classes to follow PSR-4 naming conventions and directory structure
Namespace Organization: Organized code into logical modules under OneMedia\Modules*
Important
Large files such as class-admin, class-hook, class-brand-admin, along with many global helpers, have been broken down and reorganized into MediaSharing and MediaLibrary modules based on their responsibility. Media-sharing (governing-site) logic lives in MediaSharing, while
Media Libraryspecific logic lives in MediaLibrary.There are a few cases of overlapping usage across both modules. I’ve cleaned this up as much as possible. Looking for suggestions for further improvements.
Checklist