tengattack / eslint-plugin-php-markup

A eslint plugin to process PHP markup
8 stars 2 forks source link

lost php code after auto-fixing #2

Open kkmuffme opened 5 years ago

kkmuffme commented 5 years ago

when we have like "<?php ... ?>" then the plugin doesn't work & eslint-html gives: 8:46 error Parsing error: Unexpected token php

e.g.

<script>
function valueAddedToCart( final_result ) {
    fbq( 'track', 'AddToCart', {
        'content_ids': "<?php echo $product_id; ?>",
    });
}
</script>
tengattack commented 5 years ago

It should be work now. BTW, I suggest using <?= ... ?> for those echo only statements. And we can using custom replacement for <?= ... ?> through eslintrc settings:

{
  // ...
  "settings": {
    "php/markup-replacement": {
      "php": "",
      "=": "0",   // `<?= ... ?>` would be replaced by `=`'s value
    },
  },
  // ...
}
kkmuffme commented 5 years ago

Thanks a lot for the quick reply & quick fix.Works perfectly now

tengattack commented 5 years ago

Thank you for providing issues :)

kkmuffme commented 5 years ago

Sorry to reopen, but there is a major bug now in v0.2.1: It now completely removes <?php tags without adding them back in some cases:

<script>
function valueAddedToCart( final_result ) {
    <?php 
    if ( $someVar == $otherVar ) { ?>
        fbq( 'track', 'AddToCart', {
            'content_ids': '<?php echo $product_id; ?>',
        });
    <?php } ?>
}
</script>

will result in this:

<script>
function valueAddedToCart( final_result ) {
    fbq( 'track', 'AddToCart', {
        'content_ids': '<?php echo $product_id; ?>'
    });
<?php } ?>
}
</script>
tengattack commented 5 years ago

Ummm, is that auto-fix result?

BTW, could you provide a minimal implementation (.eslintrc, problem scripts, ...)

kkmuffme commented 5 years ago

Yes.

Command I ran: eslint --config /mypath/.eslintrc.json --fix /mypath/example.php

My file:

<?php
echo 'hello';
?>
<script>
function valueAddedToCart( final_result ) {
    <?php 
    if ( $someVar == $otherVar ) { ?>
        fbq( 'track', 'AddToCart', {
            'content_ids': '<?php echo $product_id; ?>',
        });
    <?php } ?>
}
</script>

<?php
echo 'abc';

Here is my versions/config: (using 0.2.1 of php-markup)

{
    "plugins": [
        "html",
        "php-markup"
    ],
    "settings": {
        "html/indent": "+tab",
        "html/report-bad-indent": "error"
    },
    "env": {
        "browser": true,
        "worker": true,
        "phantomjs": true,
        "qunit": true,
        "jquery": true,
        "serviceworker": true,
        "es6": true
    },
    "extends": "eslint:recommended",
    "globals": {
        "_": false,
        "e": false,
        "Backbone": false,
        "JSON": false,
        "wp": false,
        "ajax_url": true,
        "ajaxurl": true,
        "fbq": true,
        "dataLayer": true
    },
    "rules": {
        "dot-notation": [
            "error",
            {
                "allowKeywords": true,
                "allowPattern": "^[a-z]+(_[a-z]+)+$"
            }
        ],
        "array-bracket-spacing": [
            "error",
            "always"
        ],
        "linebreak-style": [
            "error",
            "unix"
        ],
        "lines-around-comment": [
            "error",
            {
                "beforeLineComment": true,
                "allowBlockStart": true,
                "allowBlockEnd": true,
                "allowClassStart": true,
                "allowObjectStart": true,
                "allowArrayStart": true,
                "allowArrayEnd": true
            }
        ],
        "one-var-declaration-per-line": [
            "error",
            "initializations"
        ],
        "accessor-pairs": "error",
        "arrow-spacing": [
            "error",
            {
                "before": true,
                "after": true
            }
        ],
        "block-spacing": [
            "error",
            "always"
        ],
        "brace-style": [
            "error",
            "1tbs",
            {
                "allowSingleLine": true
            }
        ],
        "comma-dangle": [
            "error",
            {
                "arrays": "never",
                "objects": "never",
                "imports": "never",
                "exports": "never",
                "functions": "never"
            }
        ],
        "comma-spacing": [
            "error",
            {
                "before": false,
                "after": true
            }
        ],
        "comma-style": [
            "error",
            "last"
        ],
        "curly": [
            "error",
            "all"
        ],
        "dot-location": [
            "error",
            "property"
        ],
        "eol-last": "error",
        "eqeqeq": [
            "error",
            "smart"
        ],
        "func-call-spacing": [
            "error",
            "never"
        ],
        "generator-star-spacing": [
            "error",
            {
                "before": true,
                "after": true
            }
        ],
        "indent": [
            "error",
            "tab"
        ],
        "key-spacing": [
            "error",
            {
                "beforeColon": false,
                "afterColon": true
            }
        ],
        "keyword-spacing": [
            "error",
            {
                "before": true,
                "after": true
            }
        ],
        "no-eval": "error",
        "no-extend-native": "error",
        "no-extra-bind": "error",
        "no-extra-parens": [
            "error",
            "functions"
        ],
        "no-iterator": "error",
        "no-label-var": "error",
        "no-labels": [
            "error",
            {
                "allowLoop": false,
                "allowSwitch": false
            }
        ],
        "no-lone-blocks": "error",
        "no-mixed-operators": [
            "error",
            {
                "groups": [
                    [
                        "==",
                        "!=",
                        "===",
                        "!==",
                        ">",
                        ">=",
                        "<",
                        "<="
                    ],
                    [
                        "&&",
                        "||"
                    ],
                    [
                        "in",
                        "instanceof"
                    ]
                ],
                "allowSamePrecedence": true
            }
        ],
        "no-mixed-spaces-and-tabs": "error",
        "no-multi-spaces": "error",
        "no-multi-str": "error",
        "no-multiple-empty-lines": [
            "error",
            {
                "max": 1,
                "maxEOF": 0
            }
        ],
        "no-new": "error",
        "no-new-func": "error",
        "no-new-object": "error",
        "no-new-require": "error",
        "no-new-symbol": "error",
        "no-new-wrappers": "error",
        "no-octal-escape": "error",
        "no-path-concat": "error",
        "no-proto": "error",
        "no-return-assign": [
            "error",
            "except-parens"
        ],
        "no-return-await": "error",
        "no-self-compare": "error",
        "no-sequences": "error",
        "no-shadow-restricted-names": "error",
        "no-template-curly-in-string": "error",
        "no-throw-literal": "error",
        "no-trailing-spaces": "error",
        "no-undef-init": "error",
        "no-unmodified-loop-condition": "error",
        "no-unneeded-ternary": [
            "error",
            {
                "defaultAssignment": false
            }
        ],
        "no-unused-expressions": [
            "error",
            {
                "allowShortCircuit": true,
                "allowTernary": true,
                "allowTaggedTemplates": true
            }
        ],
        "no-use-before-define": [
            "error",
            {
                "functions": false,
                "classes": false,
                "variables": false
            }
        ],
        "no-useless-call": "error",
        "no-useless-computed-key": "error",
        "no-useless-constructor": "error",
        "no-useless-escape": "error",
        "no-useless-rename": "error",
        "no-useless-return": "error",
        "no-whitespace-before-property": "error",
        "no-with": "error",
        "operator-linebreak": [
            "error",
            "none"
        ],
        "padded-blocks": [
            "error",
            {
                "blocks": "never",
                "switches": "never",
                "classes": "never"
            }
        ],
        "quotes": [
            "error",
            "single",
            {
                "avoidEscape": true,
                "allowTemplateLiterals": true
            }
        ],
        "semi": [
            "error",
            "always"
        ],
        "semi-spacing": [
            "error",
            {
                "before": false,
                "after": true
            }
        ],
        "space-before-blocks": [
            "error",
            "always"
        ],
        "space-before-function-paren": [
            "error",
            "never"
        ],
        "space-in-parens": [
            "error",
            "always",
            {
                "exceptions": [
                    "{}",
                    "[]"
                ]
            }
        ],
        "space-infix-ops": "error",
        "space-unary-ops": [
            "error",
            {
                "words": true,
                "nonwords": false
            }
        ],
        "unicode-bom": [
            "error",
            "never"
        ],
        "wrap-iife": [
            "error",
            "any",
            {
                "functionPrototypeMethods": true
            }
        ],
        "yield-star-spacing": [
            "error",
            "both"
        ],
        "yoda": [
            "error",
            "never"
        ]
    }
}
tengattack commented 5 years ago

It is not recommended to do auto-fix with processors. The source code will transform to following code first (with default settings, and which is php do):

function valueAddedToCart( final_result ) {
            fbq( 'track', 'AddToCart', {
            'content_ids': '0'
        });
    }

And auto fix will do:

-           fbq( 'track', 'AddToCart', {
+       fbq( 'track', 'AddToCart', {

After transforming back to original source code, it will found that parts of code delete and apply to php code here.

If you using upgrade to v0.2.3, please try using setting:

{
  "settings": {
    "php/remove-whitespace": true 
  }
}

And it works, but it may not always work.

kkmuffme commented 5 years ago

Thanks, still the same bug with the latest version though (with remove-whitespace setting set to true)

tengattack commented 5 years ago

with php/remove-whitespace true,

The example fixed in my environment

 <?php
 echo 'hello';
 ?>
 <script>
 function valueAddedToCart( final_result ) {
    <?php
    if ( true ) { ?>
-           fbq( 'track', 'AddToCart', {
+       fbq( 'track', 'AddToCart', {
            'content_ids': '<?php echo 0; ?>'
        });
    <?php } ?>
 }
 </script>

 <?php
 echo 'abc';
kkmuffme commented 5 years ago

thanks for your test, I now tested it again & found the issue is due to the indentation:

This works perfectly (I get the correct result after eslint):

<?php

echo 'hello';
?>
<script>
function valueAddedToCart( final_result ) {
    <?php if ( true ) { ?>
    fbq( 'track', 'AddToCart', {
        'content_ids': '<?php echo $product_id; ?>',
    });
    <?php } ?>
}
</script>

<?php
echo 'abc';

But when I have the js incorrectly indented, it breaks:

ORIGINAL:
<?php

echo 'hello';
?>
<script>
function valueAddedToCart( final_result ) {
    <?php if ( true ) { ?>
        fbq( 'track', 'AddToCart', {
            'content_ids': '<?php echo $product_id; ?>',
        });
    <?php } ?>
}
</script>

<?php
echo 'abc';

will result in this where 1 of the <?php ... ?> is missing: (btw: how did you do this in the markdown to highlight the removed/added lines red/green)

AFTER ESLINT WITH --fix
<?php

echo 'hello';
?>
<script>
function valueAddedToCart( final_result ) {
    fbq( 'track', 'AddToCart', {
        'content_ids': '<?php echo $product_id; ?>'
    });
    <?php } ?>
}
</script>

<?php
echo 'abc';
tengattack commented 5 years ago

Haha, you can specify language for markdown code by following language name after block code first line:

```diff

I’m using diff to render the diff style

tengattack commented 5 years ago

Back to the topic, I think it’s difficult to identify the fix range including the original source code. So I suggest to use it in IDE instead of auto fix, it can help to you to found problems.

kkmuffme commented 5 years ago

ah thanks with the diff

Yes I disabled auto fix now for php files.

If you have time, would be great if you could take a look at it though, as it seems the issue is left only when it reindents js that is directly after a php block

(auto fixing saves lots of time so I'd prefer to use it for php files too)

tengattack commented 5 years ago

I changed the title for more precise description of the problem :)

kkmuffme commented 5 years ago

Just tested with 0.2.4 with this config:

        "php/markup-replacement": {"php": "0", "=": "0"},
        "php/keep-eol": false,
        "php/remove-whitespace": true,
        "php/remove-empty-line": true

but still the same issue

tengattack commented 5 years ago

yep, the option php/remove-empty-line could not fix this problem, the fixes trying to transform the deletion code will result in the loss of original source code.

kkmuffme commented 5 years ago

Ok I found a solution (hack?) that works.

config:

"php/markup-replacement": {"php": "lintGlobal = 0;", "=": "lintGlobal = 0;"}, 
"php/keep-eol": true,
"php/remove-whitespace": true,
"php/remove-empty-line": false

Additionally I set this as global in the eslintrc file: "lintGlobal": true,

only with this (I tried every other combination) I get correct results:

Reason it works: php code is replaced with valid js code, then gets correctly linted & then replaced back. Empty lines (= lines with only php code) do NOT get removed, but also replaced by this, which makes it work correctly (no php code removed) for php only lines. When within quotes e.g. 'content_ids': '<?php echo $product_id; ?>' we can put anything anyway, so its no issue that it's replaces with lintGlobal = 0;

tengattack commented 5 years ago

You are a genius! I think you can also use a function like lintPHPCode(); for replacement :) BTW, maybe you don't need set php/remove-whitespace to true?

kkmuffme commented 5 years ago

thanks

yes, function would be possible too, but depending on eslint settings this can get messed accidentally perhaps

yes I had it set to false anyway (just pasted the wrong thing).

IMPORTANT: do NOT put the ; in the markup-replacement.

Otherwise the ?> closing tags can get messed up in unquoted strings, e.g. var options = <?php echo json_encode( $some_var ); ?>;

would end up as

var options = <?php echo json_encode( $some_var ); ?;

Correct config is:

"php/markup-replacement": {"php": "lintGlobal = 0", "=": "lintGlobal = 0"},
"php/keep-eol": true,
"php/remove-whitespace": false,
"php/remove-empty-line": false
tengattack commented 5 years ago

Thank you for your tests.

If you set eslintrc no-semi rule, it would be better using replacement code without end with ;. However, if we need semicolon for JavaScript code, maybe auto-fix will help us to add it, and we can't add ; at the end of the replacement code for the above reason, so it will break the code consistency in those php only statement with extra semicolons. And I have no idea how to solve it right now.

Will you code be transformed to some kind of the following code?

<script>
function valueAddedToCart( final_result ) {
    <?php 
    if ( $someVar == $otherVar ) { ?>;
        fbq( 'track', 'AddToCart', {
            'content_ids': '<?php echo $product_id; ?>',
        });
    <?php } ?>;
}
</script>
tengattack commented 5 years ago

Oh, maybe we can use eslint inline comments: What about this: /* eslint-disable */ lintPHPCode() /* eslint-enable */

Or this: lintPHPCode(); /* eslint-disable-line semi */

kkmuffme commented 5 years ago

atm it seems to be working fine for me with the config I posted above.

that's a good idea with the inline comments though. I will post an update if I find any cases where my current config isn't working correctly & will try it with that suggestion

kkmuffme commented 5 years ago

// eslint-disable-line semi => will lead to it not working at all (various random errors) / eslint-disable-line semi / (or eslint-enable & disable) leads to issues when there are multiple Githubissues.

  • Githubissues is a development platform for aggregating issues.