Skip to content

Add support for comma separated shorthand taxonomies#801

Closed
kallehauge wants to merge 4 commits into
10up:developfrom
kallehauge:feature/woo-multiple-terms
Closed

Add support for comma separated shorthand taxonomies#801
kallehauge wants to merge 4 commits into
10up:developfrom
kallehauge:feature/woo-multiple-terms

Conversation

@kallehauge

Copy link
Copy Markdown
Contributor

The current implementation of handling "supported taxonomies" assumes that the query_var argument is a single value (example: 'product_brand' => 'adidas') - It does not support multiple terms separated by a comma (example: 'product_brand' => 'adidas,nike').

Comment thread features/woocommerce/woocommerce.php Outdated
$integrate = true;

$terms = array( $term );
$terms = explode( ',', $term );

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.

What if term is an array? Seems like we need to check that case too.

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.

See this?

@kallehauge kallehauge May 4, 2017

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.

Ah, sorry @tlovett1 - I cannot remember why I did not check if it was an array. I guess it's because the existing code assumes that it is a string.
But you're correct, there should be a check for "array".

I will correct it in the near future 👍

@kallehauge

Copy link
Copy Markdown
Contributor Author

Sorry this took so long @tlovett1 - please take a look at it again

Comment thread features/woocommerce/woocommerce.php Outdated
// If it is a string then we convert it to an array with one or more
// values to handle it with the same logic.
if ( is_string( $term ) ) {
$terms = explode( ',', $term );

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.

Seems like this only gets defined if it's a string

@tlovett1 tlovett1 removed this from the 2.4 milestone Oct 3, 2017
@kallehauge

Copy link
Copy Markdown
Contributor Author

Brain fart time – great catch @tlovett1. It should be ready for merge, this time ☺️

@kallehauge

Copy link
Copy Markdown
Contributor Author

@lukaspawlik can you confirm that this have been fixed with your work in: 41f9f18 ? ☺️

@kallehauge kallehauge closed this Nov 16, 2017
@kallehauge kallehauge reopened this Nov 28, 2018
@kallehauge

Copy link
Copy Markdown
Contributor Author

@tlovett1 @lukaspawlik I've finally updated some of our projects to the latest version of ElasticPress and discovered that this is still not covered. It's the same case as above.
I might make a patch in the following days - if not, then this just serves as a reminder :-)

@allan23

allan23 commented Feb 20, 2019

Copy link
Copy Markdown

Thank you for your interest in contributing to ElasticPress. At the time of this writing, we are focusing our development efforts on version 3.0 in the develop branch, which is an entirely different codebase. We ask that you please take some time to review the new codebase and alter your pull request as needed to accommodate these major changes.

@allan23 allan23 closed this Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants