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

Parse trigger attribute for fields in field parser #292

Merged
merged 3 commits into from May 26, 2020
Merged

Parse trigger attribute for fields in field parser #292

merged 3 commits into from May 26, 2020

Conversation

hardik-satasiya
Copy link
Contributor

@hardik-satasiya hardik-satasiya commented Dec 11, 2017

When using Dynamic Syntax parser (field parser)

{variable name="cms-page" type="dropdown" label="Cms Page" trigger="action:show|field:type|condition:value[cms-page]" }{/variable}

its parses trigger as "action:show|field:type|condition:value[cms-page]"

it should parse it as an array so when fields are rendered, trigger option can be used for JS trigger api.

@LukeTowers
Copy link
Contributor

@hardik-satasiya Please add the unit test for this change, thanks for your work on it!

@hardik-satasiya
Copy link
Contributor Author

@LukeTowers
ok i will add it and update you.

@hardik-satasiya
Copy link
Contributor Author

hardik-satasiya commented Dec 13, 2017

Hi @LukeTowers , added proper unit-test.

Although now inside static pages it shows desired output.

As a future development we can also add recursive parsing option

for example :
{variable name="field2" label="Field 2" type="text" trigger="action:enable|field:field1|condition:value[m]" }{/variable}

here action:enable is string value so it works fine but in case if you need multiple option

trigger="action:hide|empty -> it failed to parse this one (its also desired behavior as code is not meant to go 1 level more deep)

same goes for check-box

options="all|description-for-all -> i can not make "description-for-all" as description as it will be pushed in to array as new entry

so can we do something like this trigger="action:{enable|hide}|field:field1|condition:value[m]" ...

not sure how to do it or how we can start it but just an idea.

in case if we needed it or should we let it be like now.

When using Dynamic Syntax parser (field parser)
{variable name="cms-page" type="dropdown" label="Cms Page" trigger="action:show|field:type|condition:value[cms-page]" }{/variable}
its parses trigger as "action:show|field:type|condition:value[cms-page]"

it should parse it as an array so when fields are rendered, so trigger option can be used for JS trigger api.
@LukeTowers
Copy link
Contributor

@hardik-satasiya could you provide a bit more information on your suggestion of not processing arrays more than one level deep? Examples of valid YAML configurations that are impossible under the current parser implementation would be perfect. Thanks for all your work on this so far!

@hardik-satasiya
Copy link
Contributor Author

hardik-satasiya commented Dec 13, 2017

It causing problems in dynamic syntax parsing only
for example lets take trigger api attribute in dynamic syntax parsing

$content = '{variable name="field2" label="Field 2" type="text" trigger="action:hide|field:field1|condition:value[m]" }{/variable}';    
$result = \October\Rain\Parse\Syntax\FieldParser::parse($content);
$fields = $result->getFields();

echo "<pre>";
print_r($fields);
exit();

generates expected out put

Array(
    [field2] => Array
        (
            [label] => Field 2
            [type] => text
            [trigger] => Array
                (
                    [action] => hide
                    [field] => field1
                    [condition] => value[m]
                )
            [default] => 
            [X_OCTOBER_IS_VARIABLE] => 1
        )
)

according to js trigger api https://octobercms.com/docs/ui/input-trigger
Multiple actions are supported:

data-trigger-action="hide|empty"

if i want to take benefit of it here then I can try this

$content = '{variable name="field2" label="Field 2" type="text" trigger="action:hide|empty|field:field1|condition:value[m]" }{/variable}';    
$result = \October\Rain\Parse\Syntax\FieldParser::parse($content);
$fields = $result->getFields();

echo "<pre>";
print_r($fields);
exit();

It will generate config like this

Array(
    [field2] => Array
        (
            [label] => Field 2
            [type] => text
            [trigger] => Array
                (
                    [action] => hide
                    [1] => empty
                    [field] => field1
                    [condition] => value[m]
                )
            [default] => 
            [X_OCTOBER_IS_VARIABLE] => 1
        )
)

according to parsing its 100% correct it has to be same as shown but

but for us its not what we are expecting

see parser treat action's values as another array so I was wondering how can I go one level more so trigger="action:hide|empty|field:field1" can be written as trigger="action:{hide|empty}|field:field1" and we get expected out put as last one as its correct what we wanted.

expected out put is

Array(
    [field2] => Array
        (
            [label] => Field 2
            [type] => text
            [trigger] => Array
                (
                    [action] => Array(
                        [0] => hide
                        [1] => empty
                    )
                    [field] => field1
                    [condition] => value[m]
                )
            [default] => 
            [X_OCTOBER_IS_VARIABLE] => 1
        )
)

@LukeTowers
Copy link
Contributor

So are you saying that writing trigger="action:{hide|empty}|field:field1" currently does what you're saying about putting hide & empty in action's array? Or does it not, and that is merely a suggested syntax implementation?

@hardik-satasiya
Copy link
Contributor Author

hardik-satasiya commented Dec 13, 2017

its not working , that is suggested syntax, (its not implemented but we can implement that stuff for now its not in code base so)

@LukeTowers
Copy link
Contributor

Would that syntax conflict with anything else?

@hardik-satasiya
Copy link
Contributor Author

hardik-satasiya commented Dec 14, 2017

I am not sure about this, but these all examples are generated by using this commit's code changes so if you check, include this commit's changes as well.

if i am using this syntax trigger="action:{hide|empty}|field:field1... then generated output is :

$content = '{variable name="field2" label="Field 2" type="text" trigger="action:{hide|empty}|field:field1|condition:value[m]" }{/variable}';    
$result = \October\Rain\Parse\Syntax\FieldParser::parse($content);
$fields = $result->getFields();

echo "<pre>";
print_r($fields);
exit()

generated output is this

Array
(
    [field2] => Array
        (
            [label] => Field 2
            [type] => text
            [trigger] => Array
                (
                    [action] => {hide
                    [1] => empty}
                    [field] => field1
                    [condition] => value[m]
                )
            [default] => 
            [X_OCTOBER_IS_VARIABLE] => 1
        )
)

you can see in output {hide and [1] => empty} it was treated like they are array key

the main role for parsing attribute is inside this function :

october/rain/src/Parse/Syntax/FieldParser.php

protected function processOptionsToArray($optionsString)

In this passed $optionsString is => action:{hide|empty}|field:field1|condition:value[m]

so all data is exploded by pipe sign

$options = explode('|', $optionsString);

then within that it will fine ':' for making key => value pair if it doesn't find key then it will simply put numeric key.

so final output is :

Array
(
    [action] => {hide
    [1] => empty}
    [field] => field1
    [condition] => value[m]
)

we can enhance this processOptionsToArray function so it can go recursively and identify based on tags "{ }" its array and it required to be placed inside recursively

so I don't think its affected anything else, as well when static page's layout is parsed it is using regx to parse template and extract this fields then pass this fields to field parser to parse so, it should not affect anything else.

but still i am not 100% sure about it will affect anywhere else.

@LukeTowers
Copy link
Contributor

@hardik-satasiya if you could implement the {value} syntax to mean that it contains another array recursively, add unit tests, and also add documentation for it to the docs repo, that would be absolutely amazing. Thanks again for all your work on this so far!

@hardik-satasiya
Copy link
Contributor Author

Ok, sure i will work on that and let you know, thanks for your assistance.

@LukeTowers LukeTowers added this to the v1.0.466 milestone May 26, 2020
@LukeTowers LukeTowers merged commit 0ac39ce into octobercms:develop May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants