Skip to content

Sync: check schema is available or not#8143

Merged
kraftbj merged 3 commits intoAutomattic:masterfrom
Umangvaghela:issue_8137
Nov 10, 2017
Merged

Sync: check schema is available or not#8143
kraftbj merged 3 commits intoAutomattic:masterfrom
Umangvaghela:issue_8137

Conversation

@Umangvaghela
Copy link
Copy Markdown
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 This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Nov 10, 2017
Copy link
Copy Markdown
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.

// 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
Copy Markdown
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' ) )
) {

// 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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above.

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
Copy Markdown
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 need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Nov 10, 2017
Copy link
Copy Markdown
Contributor

@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.


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

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.


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

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
Copy Markdown
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 .


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

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!

Copy link
Copy Markdown
Contributor

@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.

}

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

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
Copy Markdown
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
Copy Markdown
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 need more details from you. This label will be auto-added until the PR meets all requirements. labels Nov 10, 2017
@zinigor
Copy link
Copy Markdown
Contributor

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
oskosk pushed a commit that referenced this pull request Nov 28, 2017
* 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
@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

Labels

Bug When a feature is broken and / or not performing as intended [Package] Sync [Pri] Normal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Undefined index in class.jetpack-sync-functions.php

5 participants