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.

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

@set directive cannot have value with commas. #28

Closed
jtskjelfjord opened this issue Dec 2, 2019 · 3 comments
Closed

@set directive cannot have value with commas. #28

jtskjelfjord opened this issue Dec 2, 2019 · 3 comments
Assignees
Labels
bug Something isn't working todo This feature or request is on the todo list.

Comments

@jtskjelfjord
Copy link

jtskjelfjord commented Dec 2, 2019

When using the @set with a value that contains commas, like the following examples...

@set($product_name, apply_filters('woocommerce_cart_item_name', $_product->get_name(), $cart_item, $cart_item_key))

or...

@set($product_name, "string, with, commas")

will result in....

<?php $_product = apply_filters('woocommerce_cart_item_product'; ?>

...which has unfinished/unclosed parenthesis and all $args missing.

or like this...

<?php $product_name = "string; ?>

...which is missing the last double quote and the rest of the string.

My suggestion would be to add a third parameter in explode() for Util::parse() function like this...

public static function parse($expression)
{
    return collect(explode(',', $expression, 2))
        ->map(function ($item) {
            return trim($item);
        });
}

Adding 2 as the third parameter for explode() will make it so that it only explodes on the first comma.

Can confirm that this works with the two examples provided above.

Alternatively make a new second util method to prevent issues with existing directives that requires explode on all commas.

@Log1x
Copy link
Owner

Log1x commented Dec 2, 2019

I experienced this recently myself when trying to do @set($example, ['value', 'value']) – thanks for the tip on explode(), I didn't even know it had a third parameter if I'm being brutally honest lol.

I will make it so parse() can accept a bool as a second param to handle this situation. Would that be sane?

@Log1x Log1x added bug Something isn't working todo This feature or request is on the todo list. labels Dec 2, 2019
@Log1x Log1x self-assigned this Dec 2, 2019
Log1x added a commit that referenced this issue Dec 2, 2019
… an attachment URL (Fixes #24)

bugfix(set): allow the second parameter to accept a value containing commas (e.g. an array) (Fixes #28)
enhancement(utilities): add `limit` param to the `parse` method to allow passing a limit to `explode()` (#28)
docs(update): add `@image` example for `raw`
@Log1x
Copy link
Owner

Log1x commented Dec 2, 2019

I've pushed a next branch that adds $limit to the parse() method. I will probably go ahead and publish a release now, but there are more than likely other directives that would benefit from this being set accordingly.

If you're at all interested in doing a PR later on, that'd be awesome.

@Log1x Log1x closed this as completed Dec 2, 2019
Log1x added a commit that referenced this issue Dec 2, 2019
* feature(image): allow passing `raw` as the second parameter to return an attachment URL (Fixes #24)
* bugfix(set): allow the second parameter to accept a value containing commas (e.g. an array) (Fixes #28)
* enhancement(utilities): add `limit` param to the `parse` method to allow passing a limit to `explode()` (#28)
docs(update): add `@image` example for `raw`
* chore(deps): Bump lock files
@jtskjelfjord
Copy link
Author

jtskjelfjord commented Dec 3, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working todo This feature or request is on the todo list.
Projects
None yet
Development

No branches or pull requests

2 participants