Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Sync: check schema is available or not #8143

Merged
merged 3 commits into from
Nov 10, 2017
Merged

Sync: check schema is available or not #8143

merged 3 commits into from
Nov 10, 2017

Conversation

Umangvaghela
Copy link
Contributor

Fixes #8137

Use empty() to check if it is available.

@Umangvaghela Umangvaghela requested a review from a team as a code owner November 10, 2017 06:10
@jeherve jeherve changed the title check schema is available or not Sync: check schema is available or not Nov 10, 2017
@jeherve jeherve added [Package] Sync [Pri] Normal [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Bug When a feature is broken and / or not performing as intended labels Nov 10, 2017
jeherve
jeherve reviewed Nov 10, 2017
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution.

Some remarks about the changes you've introduced in this PR.

sync/class.jetpack-sync-functions.php Outdated
@@ -47,19 +49,23 @@ public static function sanitize_taxonomy( $taxonomy ) {
}
// Remove any meta_box_cb if they are not the default wp ones.
if ( isset( $cloned_taxonomy->meta_box_cb ) &&
! in_array( $cloned_taxonomy->meta_box_cb, array( 'post_tags_meta_box', 'post_categories_meta_box' ) ) ) {
! in_array( $cloned_taxonomy->meta_box_cb, array( 'post_tags_meta_box', 'post_categories_meta_box' ) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you meant to clarify the if statement here, it might be best to move the first check on its own line as well:

if (
    isset( $cloned_taxonomy->meta_box_cb )
    && ! in_array( $cloned_taxonomy->meta_box_cb, array( 'post_tags_meta_box', 'post_categories_meta_box' ) )
) {

sync/class.jetpack-sync-functions.php Outdated
$cloned_taxonomy->meta_box_cb = null;
}
// Remove update call back
if ( isset( $cloned_taxonomy->update_count_callback ) &&
! is_null( $cloned_taxonomy->update_count_callback ) ) {
! is_null( $cloned_taxonomy->update_count_callback )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

sync/class.jetpack-sync-functions.php Outdated
if ( isset( $cloned_taxonomy->rest_controller_class ) &&
'WP_REST_Terms_Controller' !== $cloned_taxonomy->rest_controller_class ) {
if ( isset( $cloned_taxonomy->rest_controller_class ) &&
'WP_REST_Terms_Controller' !== $cloned_taxonomy->rest_controller_class
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 10, 2017
zinigor
zinigor requested changes Nov 10, 2017
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing! I have added several comments, please address them.

sync/class.jetpack-sync-functions.php Outdated
@@ -203,26 +210,26 @@ public static function get_protocol_normalized_url( $callable, $new_value ) {
$option_key = self::HTTPS_CHECK_OPTION_PREFIX . $callable;

$parsed_url = wp_parse_url( $new_value );
if ( ! $parsed_url ) {
if ( empty ( $parsed_url['scheme'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't just return here if there's no schema set for the URL. To avoid warnings let's instead check against empty down there where we first try to access the scheme key.

sync/class.jetpack-sync-functions.php Outdated
return $new_value;
}

$scheme = $parsed_url['scheme'];
$scheme_history = get_option( $option_key, array() );
$scheme = $parsed_url['scheme'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we need to see if the key exists, and I'd prefer to use array_key_exists here instead of empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zinigor
If I use
if ( array_key_exists ( 'scheme' , $parsed_url) ) {
$scheme = $parsed_url['scheme'];
} else {
$scheme = '';
}
This is fine .

sync/class.jetpack-sync-functions.php Outdated

return set_url_scheme( $new_value, $forced_scheme );
}

public static function get_raw_url( $option_name ) {
$value = null;
$value = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid unnecessary changes like this. It's OK to remove spaces that shouldn't be there in related code, but these edits make the PR harder to review. Thanks!

zinigor
zinigor requested changes Nov 10, 2017
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor thing, and this will be good to go, thank you.

sync/class.jetpack-sync-functions.php Outdated
@@ -206,8 +206,11 @@ public static function get_protocol_normalized_url( $callable, $new_value ) {
if ( ! $parsed_url ) {
return $new_value;
}

$scheme = $parsed_url['scheme'];
if( array_key_exists ( 'scheme' , $parsed_url) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after if and after $parsed_url, please fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, Sir

@Umangvaghela
Copy link
Contributor Author

@zinigor
I add the change which is given to you.

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 10, 2017
@zinigor
Copy link
Member

zinigor commented Nov 10, 2017

This is ready to go! Thank you.
Note: when merging please squash to get rid of the whitespace changes.

@kraftbj kraftbj merged commit 38be496 into Automattic:master Nov 10, 2017
@oskosk oskosk added this to the 5.6 milestone Nov 22, 2017
jeherve added a commit that referenced this pull request Nov 24, 2017
@jeherve
Changelog: add #8143
ad665d9
oskosk pushed a commit that referenced this pull request Nov 28, 2017
@jeherve @oskosk
Changelog 5.6: running changelog. (#8192)
7844644 
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
@oskosk oskosk mentioned this pull request Feb 14, 2018
Closed
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve left review comments

@zinigor zinigor zinigor approved these changes

Assignees
No one assigned
Labels
[Package] Sync [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
5.6
Development

Successfully merging this pull request may close these issues.

Undefined index in class.jetpack-sync-functions.php
5 participants
@Umangvaghela @zinigor @jeherve @kraftbj @oskosk