feat: PHPCS: adding support for the WordPress VIP Go standard#2482
feat: PHPCS: adding support for the WordPress VIP Go standard#2482jasonbahl merged 2 commits intowp-graphql:developfrom
Conversation
| if ( $wp_rewrite->use_verbose_page_rules && preg_match( '/pagename=\$matches\[([0-9]+)\]/', $query, $varmatch ) ) { | ||
| // This is a verbose page match, let's check to be sure about it. | ||
| $page = get_page_by_path( $matches[ $varmatch[1] ] ); | ||
| $page = get_page_by_path( $matches[ $varmatch[1] ] ); // phpcs:ignore WordPressVIPMinimum.Functions.RestrictedFunctions.get_page_by_path_get_page_by_path |
There was a problem hiding this comment.
For this one, should we do a if/else strategy with wpcom_vip_get_page_by_path?
| */ | ||
| public static function get_raw_data() { | ||
| $input = file_get_contents( 'php://input' ); | ||
| $input = file_get_contents( 'php://input' ); // phpcs:ignore WordPressVIPMinimum.Performance.FetchingRemoteData.FileGetContentsRemoteFile |
There was a problem hiding this comment.
VIP recommends:
file_get_contents()is highly discouraged for remote requests, please usewpcom_vip_file_get_contents()orvip_safe_wp_remote_get()
| case 'source_url': | ||
| $url = $args['id']; | ||
| $post_id = attachment_url_to_postid( $url ); | ||
| $post_id = attachment_url_to_postid( $url ); // phpcs:ignore WordPressVIPMinimum.Functions.RestrictedFunctions.attachment_url_to_postid_attachment_url_to_postid |
There was a problem hiding this comment.
Another one where we could do a if/else strategy with wpcom_vip_attachment_url_to_postid
|
Code Climate has analyzed commit d62e765 and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
There was a problem hiding this comment.
Awesome work @renatonascalves !
Re adopting vipcom_ functions, not saying we should, but if we do:
- let's wrap the
if( method_exists( 'vipcom_fn') {return vipcom_fn();} return wp_fn();logic in a separate class (Utils\Plugabbleor something) to keep things DRY.
Edit: we also might want to add a hook, or drop the namespace and wrap each method in its ownmethod_exists(), so we can provide the same first-party parity with non-WPVip setups or plugins likewp-graphql-smart-cache - A lot of those methods employ caching, so we should make sure they're unit tested to avoid issues we had with the ObjectCursor in #2457
- Anything in
NodeResolver::resolve_uri()should be left untouched (ie errors suppressed, no code change) until #2342 is merged. Currently most of it is lifted directly fromWP:parse_request(), isn't unit tested, and is a source for a plethora of bugs ( 👀 #2366 ), so we should avoid changing it unless absolutely necessary.
|
@justlevine May I suggest then to move on silencing those examples in this pull request? I'll create issues to address those separate changes. Introducing unit tests, for example, would increase the scope a little bit since we would need to add support for the VIP |
|
@jasonbahl Could you code review this pull request when you have a chance? |
|
@renatonascalves I think I agree with @justlevine about adding vip support in a separate file. If any of these functions can't be filtered and replaced with the VIP equivalent, it's probably a good candidate for a new filter. Ideally, if do need to introduce new filters, I'd like to keep them central when possible. For example (doesn't apply here, but just to set the tone), if we needed to filter something in the UserLoader, if possible I'd prefer the |
|
Thank you! I'll proceed to merge this and create issues for the follow-up work. Do you guys know what's this Check code (8.0.15) Github Action about? |
|
@renatonascalves I think I need to update the required checks. I think it's tied to an old PHP version that was updated. Looking now 👀 |
|
@renatonascalves ya, it was related to a change on the Github Workflow that checks PHPStan. I updated the branch settings. Good to go now. |
|
Thank you, Jason! |
What does this implement/fix? Explain your changes.
automattic/vipwpcsDoes this close any currently open issues?
fixes #1535
Any relevant logs, error output, GraphiQL screenshots, etc?
(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)
Any other comments?
Code reviewer: see my comments for some questions/considerations.