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

Make getting redirect options more flexible #271

Merged
merged 7 commits into from Jul 31, 2017

Conversation

2 participants
@diedexx
Contributor

diedexx commented Jul 28, 2017

I'm currently trying to fetch meta data of a translated version of a post.

I'm using an instance of the MLP_Language_Negotiation for this.
$validator                     = new \Mlp_Language_Header_Validator();
$parser                        = new \Mlp_Accept_Header_Parser( $validator );
$api                           = apply_filters( 'mlp_language_api', null );
// ...
$language_negotiation_instance = new \Mlp_Language_Negotiation( $api, $parser );

With the Language_negotiation I get the best matching translation for a page, which is not the page I'm currently on), for which I want to fetch meta data.

What's Included in This Pull Request

  • Adds the target_content_id to the redirect matches so it can be stored or used in API calls.
  • Adds the possibility to pass args to get_redirect_match (in my case to fetch the redirect matches for any post, not just the current page)
Show outdated Hide outdated src/inc/redirect/Mlp_Language_Negotiation.php
$translations = $this->language_api->get_translations(
array ( 'include_base' => TRUE )
wp_parse_args( $args, array( 'include_base' => true ) )

This comment has been minimized.

@tfrommen

tfrommen Jul 28, 2017

Contributor

Since we don't allow for a query string, please use array_merge() here, as it is a core PHP function and thus doesn't need to be mocked in unit tests. Also, the uppercase TRUE is actually correct - well, in MultilingualPress v2, at least. 😀

@tfrommen

tfrommen Jul 28, 2017

Contributor

Since we don't allow for a query string, please use array_merge() here, as it is a core PHP function and thus doesn't need to be mocked in unit tests. Also, the uppercase TRUE is actually correct - well, in MultilingualPress v2, at least. 😀

Show outdated Hide outdated src/inc/redirect/Mlp_Language_Negotiation.php
'url' => '',
'language' => '',
'site_id' => 0,
'target_content_id' => 0,

This comment has been minimized.

@tfrommen

tfrommen Jul 28, 2017

Contributor

Let's call this just content_id.

@tfrommen

tfrommen Jul 28, 2017

Contributor

Let's call this just content_id.

This comment has been minimized.

@diedexx

diedexx Jul 28, 2017

Contributor

First of all, thanks for you feedback!

What do you think of post_id instead?

@diedexx

diedexx Jul 28, 2017

Contributor

First of all, thanks for you feedback!

What do you think of post_id instead?

This comment has been minimized.

@tfrommen

tfrommen Jul 28, 2017

Contributor

No, content_id is good, because it's not only used for posts. If you visit a term archive, the ID refers to the term taxonomy ID, for example.

Thanks for updating the PR. Could you take care of the indentation in the two arrays, then I'll merge this right away.

@tfrommen

tfrommen Jul 28, 2017

Contributor

No, content_id is good, because it's not only used for posts. If you visit a term archive, the ID refers to the term taxonomy ID, for example.

Thanks for updating the PR. Could you take care of the indentation in the two arrays, then I'll merge this right away.

Show outdated Hide outdated src/inc/redirect/Mlp_Language_Negotiation.php
'url' => $url,
'language' => $language->get_name( 'http' ),
'site_id' => $site_id,
'target_content_id' => $translation->get_target_content_id(),

This comment has been minimized.

@tfrommen

tfrommen Jul 28, 2017

Contributor

Again, content_id, of course.

@tfrommen

tfrommen Jul 28, 2017

Contributor

Again, content_id, of course.

@tfrommen tfrommen merged commit 4bfe7a3 into inpsyde:2.7 Jul 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Jul 31, 2017

Contributor

Thanks, @diedexx!

Contributor

tfrommen commented Jul 31, 2017

Thanks, @diedexx!

Dinamiko added a commit that referenced this pull request Jul 31, 2017

@Dinamiko Dinamiko referenced this pull request Jul 31, 2017

Merged

Add #271 to v3 #272

@diedexx

This comment has been minimized.

Show comment
Hide comment
@diedexx

diedexx Aug 2, 2017

Contributor

Hi, I realised I set the base of this PR to 2.7, however, that version was already released at that time. 😅
Can this move to 2.8 or is this branch also used for 2.7.1? Also, is there a scheduled release date?

Contributor

diedexx commented Aug 2, 2017

Hi, I realised I set the base of this PR to 2.7, however, that version was already released at that time. 😅
Can this move to 2.8 or is this branch also used for 2.7.1? Also, is there a scheduled release date?

@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Aug 2, 2017

Contributor

Yes, that's fine. The version will be 2.7.1 - based off the 2.7 branch - scheduled for release ... today. 😀

Contributor

tfrommen commented Aug 2, 2017

Yes, that's fine. The version will be 2.7.1 - based off the 2.7 branch - scheduled for release ... today. 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment