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

Volt Replacing Single Quote with Double Quote #16019

Closed
kgrammer opened this issue Jul 11, 2022 · 20 comments · Fixed by #16024
Closed

Volt Replacing Single Quote with Double Quote #16019

kgrammer opened this issue Jul 11, 2022 · 20 comments · Fixed by #16024
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium

Comments

@kgrammer
Copy link

I am setting the following variables in my VOLT file:

 {% set login_gtm_class = 'class="bl_ga_tag"' %}

This was working as expected in RC2 and produced:

    <?php $login_gtm_class = 'class="bl_ga_tag"'; ?>

But now in RC3 I'm getting the following:

    <?php $login_gtm_class = "class="bl_ga_tag""; ?>

The code is replacing the single quotes with double quotes which is breaking everything.

The original behavior needs to be returned.

Details

  • Phalcon version: 5.0.0RC3
  • PHP Version: 8.1.8 (Zend Engine v4.1.8)
  • Operating System: Ubuntu 22.04
  • Installation type: Compiling from source
  • Zephir version (if any): 0.16.0
  • Server: Apache
  • Other related info (Database, table schema): N/A

Additional context
Nothing more is required.

@kgrammer kgrammer added bug A bug report status: unverified Unverified labels Jul 11, 2022
@Deathamns
Copy link
Contributor

Also double quotes now interpret PHP variables. I had to put (external) JSON data to an attribute, but one of my variable name was the part of a JSON property name. Maybe single quotes would be safer in Volt.

// oversimplified example
{% set today = date('Y-m-d') %}
<div{{ ' data-json=\"{_$today: 1}\"' }}"></div>

// translated to (which obviously leaked the value of $today, and caused parsing error in JS):
<div data-json="<?= "{_$today: 1}" ?>"></div>

@borisdelev
Copy link
Sponsor Contributor

... one more thing:
{% set divider = '<i class="right chevron icon divider"></i>' %} - this return error. When switch quotes works, but need to be fixed.

@niden niden mentioned this issue Jul 15, 2022
5 tasks
@niden niden linked a pull request Jul 15, 2022 that will close this issue
5 tasks
@niden niden self-assigned this Jul 15, 2022
@niden niden added status: medium Medium 5.0 The issues we want to solve in the 5.0 release and removed status: unverified Unverified labels Jul 15, 2022
@niden niden added this to Working on it in Phalcon Roadmap Jul 15, 2022
@niden
Copy link
Sponsor Member

niden commented Jul 19, 2022

Resolved in #16024

@niden niden closed this as completed Jul 19, 2022
Phalcon Roadmap automation moved this from Working on it to Implemented Jul 19, 2022
@fichtner
Copy link

Personally having this completely destroy volt templates generated HTML with PHP parser errors I'd think "status: medium" seems a little shy of how bad this actually was especially during RC phase.

Cheers,
Franco

@niden
Copy link
Sponsor Member

niden commented Jul 27, 2022

Personally having this completely destroy volt templates generated HTML with PHP parser errors I'd think "status: medium" seems a little shy of how bad this actually was especially during RC phase.

Cheers, Franco

Actually it will not. The latest commit reverted everything to what it was before, therefore existing templates will work as expected.

The only difference is that if one is using the TagFactory will use double quotes and only that

{{ tag.title("\t", "\n\n") }}
<?= $this->tag->title("\t", "\n\n"); >

The reason it was set to medium was because of that - it does not break backwards compatibility.

@fichtner
Copy link

@niden

Our experience differs from that:

[27-Jul-2022 07:37:17 Etc/UTC] ParseError: syntax error, unexpected identifier "readonly" in /usr/local/opnsense/mvc/app/cache/_usr_local_opnsense_mvc_app_views_layout_partials_form_input_tr.volt.php:20

The code generated was definitely wrong:

<?= ((empty($readonly) ? (false) : ($readonly)) ? "readonly="readonly"" : "") ?>

Now I don't know if that's a different issue but we reverted to RC2 and there it's fine. I guess we will see after RC4 or the release is out.

@kgrammer
Copy link
Author

I recently updated to the latest RC3 and the original single quote issue has been resolved in RC3 now.

@kgrammer
Copy link
Author

kgrammer commented Jul 27, 2022

@fichtner

I meant to add that I think in your case, the issue is a PHP parsing error and not a Phalcon bug. PHP can't handle quotes inside of quotes. The PHP parsing is ending with your second double quote.

I would change the code to the following (note single quote around 'readonly'):

<?= ((empty($readonly) ? (false) : ($readonly)) ? "readonly='readonly'" : "") ?>

I believe that will solve your error.

@fichtner
Copy link

Err, but all versions of Phalcon up until RC2 did it in a way that it didn't break the code Phalcon generated: <?= ((empty($readonly) ? (false) : ($readonly)) ? 'readonly="readonly"' : '') ?> and I don't see how this won't break in the future changing doubles to singles if nobody will implement proper escaping such as "readonly=\"readonly\"". It's not magic.

@niden niden reopened this Jul 27, 2022
@niden
Copy link
Sponsor Member

niden commented Jul 27, 2022

@fichtner Let me run a test on this. Maybe something was missed.

What is the exact volt code if you don't mind?

@niden
Copy link
Sponsor Member

niden commented Jul 27, 2022

Thank you bud. I will get that test going today and report back.

@niden
Copy link
Sponsor Member

niden commented Aug 8, 2022

@fichtner I run the whole template using RC3 and this is what I got

<tr id="row_<?= $id ?>" <?php if ((empty($advanced) ? (false) : ($advanced)) == 'true') { ?> data-advanced="true"<?php } ?>>
    <td>
        <div class="control-label" id="control_label_<?= $id ?>">
            <?php if ((empty($help) ? (false) : ($help))) { ?>
                <a id="help_for_<?= $id ?>" href="#" class="showhelp"><i class="fa fa-info-circle"></i></a>
            <?php } elseif ((empty($help) ? (false) : ($help)) == false) { ?>
                <i class="fa fa-info-circle text-muted"></i>
            <?php } ?>
            <b><?= $label ?></b>
        </div>
    </td>
    <td>
        <?php if ($type == 'text') { ?>
            <input  type="text"
                    class="form-control <?= (empty($style) ? ('') : ($style)) ?>"
                    size="<?= (empty($size) ? ('50') : ($size)) ?>"
                    id="<?= $id ?>"
                <?= ((empty($readonly) ? (false) : ($readonly)) ? 'readonly="readonly"' : '') ?>
                <?php if ((empty($hint) ? (false) : ($hint))) { ?>placeholder="<?= $hint ?>"<?php } ?>
            >
        <?php } elseif ($type == 'hidden') { ?>
            <input type="hidden" id="<?= $id ?>" class="<?= (empty($style) ? ('') : ($style)) ?>" >
        <?php } elseif ($type == 'checkbox') { ?>
            <input type="checkbox"  class="<?= (empty($style) ? ('') : ($style)) ?>" id="<?= $id ?>">
        <?php } elseif ($this->isIncluded($type, ['select_multiple', 'dropdown'])) { ?>
        <select <?php if ($type == 'select_multiple') { ?>multiple="multiple"<?php } ?>
            <?php if ((empty($size) ? (false) : ($size))) { ?>data-size="<?= $size ?>"<?php } ?>
                id="<?= $id ?>"
                class="<?= (empty($style) ? ('selectpicker') : ($style)) ?>"
            <?php if ((empty($hint) ? (false) : ($hint))) { ?>data-hint="<?= $hint ?>"<?php } ?>
                data-width="<?= (empty($width) ? ('334px') : ($width)) ?>"
                data-allownew="<?= (empty($allownew) ? ('false') : ($allownew)) ?>"
                data-sortable="<?= (empty($sortable) ? ('false') : ($sortable)) ?>"
                data-live-search="true"
            <?php if ((empty($separator) ? (false) : ($separator))) { ?>data-separator="<?= $separator ?>"<?php } ?>
        ></select><?php if ((empty($style) ? ('selectpicker') : ($style)) != 'tokenize') { ?><br /><?php } ?>
            <a href="#" class="text-danger" id="clear-options_<?= $id ?>"><i class="fa fa-times-circle"></i> <small><?= $lang->_('Clear All') ?></small></a>
            <?php if ((empty($style) ? ('selectpicker') : ($style)) == 'tokenize') { ?>&nbsp;&nbsp;<a href="#" class="text-danger" id="copy-options_<?= $id ?>"><i class="fa fa-copy"></i> <small><?= $lang->_('Copy') ?></small></a>
                &nbsp;&nbsp;<a href="#" class="text-danger" id="paste-options_<?= $id ?>" style="display:none"><i class="fa fa-paste"></i> <small><?= $lang->_('Paste') ?></small></a>
            <?php } ?>
        <?php } elseif ($type == 'password') { ?>
            <input type="password" autocomplete="new-password" class="form-control <?= (empty($style) ? ('') : ($style)) ?>" size="<?= (empty($size) ? ('50') : ($size)) ?>" id="<?= $id ?>" <?= ((empty($readonly) ? (false) : ($readonly)) ? 'readonly="readonly"' : '') ?> >
        <?php } elseif ($type == 'textbox') { ?>
            <textarea class="<?= (empty($style) ? ('') : ($style)) ?>" rows="<?= (empty($height) ? ('5') : ($height)) ?>" id="<?= $id ?>" <?= ((empty($readonly) ? (false) : ($readonly)) ? 'readonly="readonly"' : '') ?>></textarea>
        <?php } elseif ($type == 'info') { ?>
            <span  class="<?= (empty($style) ? ('') : ($style)) ?>" id="<?= $id ?>"></span>
        <?php } ?>
        <?php if ((empty($help) ? (false) : ($help))) { ?>
            <div class="hidden" data-for="help_for_<?= $id ?>">
                <small><?= $help ?></small>
            </div>
        <?php } ?>
    </td>
    <td>
        <span class="help-block" id="help_block_<?= $id ?>"></span>
    </td>
</tr>

@niden
Copy link
Sponsor Member

niden commented Aug 8, 2022

Note the line:

<?= ((empty($readonly) ? (false) : ($readonly)) ? 'readonly="readonly"' : '') ?>

I also wrote a test just for that and it passes just fine here:

niden@99925b1

@niden
Copy link
Sponsor Member

niden commented Aug 8, 2022

I am going to release a new RC today, so if you don't mind let's check this again with the new version so as not to have issues like this. Changing and refactoring volt templates is not something I want developers to do.

@fichtner
Copy link

fichtner commented Aug 8, 2022

@niden I'll give it a try as soon as the next RC is out. Thanks!

@kgrammer
Copy link
Author

kgrammer commented Aug 8, 2022

@niden Likewise, I'll also update and test when the new RC release is available!

@fichtner
Copy link

fichtner commented Aug 8, 2022

@niden RC4 looks fine from here. Will continue to monitor for the rest of the week. Thanks for your help!

@niden
Copy link
Sponsor Member

niden commented Aug 8, 2022

Thank you gents. I am closing this one for now but please report back if anything is not as it should be.

@niden niden closed this as completed Aug 8, 2022
@kgrammer
Copy link
Author

kgrammer commented Aug 9, 2022

Just wanted to close the loop by adding that my initial error has indeed been resolved in 5.0.0RC4.

As always, I will open a new ticket if anything else pops up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium
Projects
Archived in project
Phalcon Roadmap
  
Implemented
Development

Successfully merging a pull request may close this issue.

5 participants