search

moodlehq / moodle-cs

Moodle Coding Style
https://github.com/moodlehq/moodle-cs
GNU General Public License v3.0
18 stars 16 forks source link

Inserted boilerplate gets broken if there is a CRLF line break after the opening tag #176

Closed micaherne closed 3 months ago

micaherne commented 3 months ago

I'm seeing an issue with the fix for the Files.BoilerplateComment sniff where there is a Windows line ending after the opening <?php tag.

Running phpcbf with the moodle standard on this code:

<?php

gives this:

<?php
// This file is part of Moodle - https://moodle.org/
// Moodle is free software: you can redistribute it and/or modify
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle.  If not, see <https://www.gnu.org/licenses/>.

This doesn't happen if running the boilerplate sniff only (vendor/bin/phpcbf --standard=moodle --sniffs=moodle.Files.BoilerplateComment) and from looking at the processing output it looks as if it may be an interaction with moodle.Commenting.InlineComment which seems to remove the blank lines from the boilerplate, which BoilerplateComment then adds back in wrongly.

I think there may be a couple of buggy things going on here so I'll try to work out in more detail what is happening.

micaherne commented 3 months ago

I've made a PR for a fix that appears to stop this happening - #177

However, the CRLF still seems to be causing strange processing issues. Where it is an LF, the boilerplate comment sniff adds the boilerplate and the file is fixed in one pass. When the CRLF is there, there is a bunch of extra processing by various other sniffs and it takes five passes to come up with the final fixed file:

        => Fixing file: 2/2 violations remaining
---START FILE CONTENT---
1|<?php
2|
--- END FILE CONTENT ---
        moodle.Files.BoilerplateComment:237 replaced token 0 (T_OPEN_TAG on line 1) "<?php\r\n" => "<?php\r\n//·This·file·is·part·of·Moodle·-·https://moodle.org/\n//\n//·Moodle·is·free·software:·you·can·redistribute·it·and/or·modify\n//·it·under·the·terms·of·the·GNU·General·Public·License·as·published·by\n//·the·Free·Software·Foundation,·either·version·3·of·the·License,·or\n//·(at·your·option)·any·later·version.\n//\n//·Moodle·is·distributed·in·the·hope·that·it·will·be·useful,\n//·but·WITHOUT·ANY·WARRANTY;·without·even·the·implied·warranty·of\n//·MERCHANTABILITY·or·FITNESS·FOR·A·PARTICULAR·PURPOSE.··See·the\n//·GNU·General·Public·License·for·more·details.\n//\n//·You·should·have·received·a·copy·of·the·GNU·General·Public·License\n//·along·with·Moodle.··If·not,·see·<https://www.gnu.org/licenses/>.\n"
        * token 0 has already been modified, skipping *
        => Fixing file: 1/2 violations remaining [made 1 pass]...               
        * fixed 1 violations, starting loop 2 *
---START FILE CONTENT---
1|<?php
2|// This file is part of Moodle - https://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle.  If not, see <https://www.gnu.org/licenses/>.

--- END FILE CONTENT ---
        Generic.Files.LineEndings:137 replaced token 0 (T_OPEN_TAG on line 1) "<?php\r\n" => "<?php\n"
        Generic.Files.EndFileNewline:72 replaced token 14 (T_COMMENT on line 2) "//·along·with·Moodle.··If·not,·see·<https://www.gnu.org/licenses/>.\n" => "//·along·with·Moodle.··If·not,·see·<https://www.gnu.org/licenses/>.\n\r\n"
        moodle.Commenting.InlineComment:425 replaced token 2 (T_COMMENT on line 2) "//\n" => ""
        moodle.Commenting.InlineComment:425 replaced token 7 (T_COMMENT on line 2) "//\n" => ""
        moodle.Commenting.InlineComment:425 replaced token 12 (T_COMMENT on line 2) "//\n" => ""
        => Fixing file: 5/2 violations remaining [made 2 passes]...             
        * fixed 5 violations, starting loop 3 *
---START FILE CONTENT---
1|<?php
// This file is part of Moodle - https://moodle.org/
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
// GNU General Public License for more details.
// You should have received a copy of the GNU General Public License
// along with Moodle.  If not, see <https://www.gnu.org/licenses/>.

2|
--- END FILE CONTENT ---
        **** moodle.Files.BoilerplateComment:141 has possible conflict with another sniff on loop 1; caused by the following change ****
        **** replaced token 2 (T_COMMENT on line 3) "//·Moodle·is·free·software:·you·can·redistribute·it·and/or·modify\n" => "//\n" ****
        **** ignoring all changes until next loop ****
        => Fixing file: 0/2 violations remaining [made 3 passes]...             
        * fixed 0 violations, starting loop 4 *
---START FILE CONTENT---
 1|<?php
 2|// This file is part of Moodle - https://moodle.org/
 3|// Moodle is free software: you can redistribute it and/or modify
 4|// it under the terms of the GNU General Public License as published by
 5|// the Free Software Foundation, either version 3 of the License, or
 6|// (at your option) any later version.
 7|// Moodle is distributed in the hope that it will be useful,
 8|// but WITHOUT ANY WARRANTY; without even the implied warranty of
 9|// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
10|// GNU General Public License for more details.
11|// You should have received a copy of the GNU General Public License
12|// along with Moodle.  If not, see <https://www.gnu.org/licenses/>.
13|
14|
--- END FILE CONTENT ---
        moodle.Files.BoilerplateComment:141 replaced token 2 (T_COMMENT on line 3) "//·Moodle·is·free·software:·you·can·redistribute·it·and/or·modify\n" => "//\n"
        moodle.Files.BoilerplateComment:141 replaced token 3 (T_COMMENT on line 4) "//·it·under·the·terms·of·the·GNU·General·Public·License·as·published·by\n" => "//·Moodle·is·free·software:·you·can·redistribute·it·and/or·modify\n"
        moodle.Files.BoilerplateComment:141 replaced token 4 (T_COMMENT on line 5) "//·the·Free·Software·Foundation,·either·version·3·of·the·License,·or\n" => "//·it·under·the·terms·of·the·GNU·General·Public·License·as·published·by\n"
        moodle.Files.BoilerplateComment:141 replaced token 5 (T_COMMENT on line 6) "//·(at·your·option)·any·later·version.\n" => "//·the·Free·Software·Foundation,·either·version·3·of·the·License,·or\n"
        moodle.Files.BoilerplateComment:141 replaced token 6 (T_COMMENT on line 7) "//·Moodle·is·distributed·in·the·hope·that·it·will·be·useful,\n" => "//·(at·your·option)·any·later·version.\n"
        moodle.Files.BoilerplateComment:141 replaced token 7 (T_COMMENT on line 8) "//·but·WITHOUT·ANY·WARRANTY;·without·even·the·implied·warranty·of\n" => "//\n"
        moodle.Files.BoilerplateComment:141 replaced token 8 (T_COMMENT on line 9) "//·MERCHANTABILITY·or·FITNESS·FOR·A·PARTICULAR·PURPOSE.··See·the\n" => "//·Moodle·is·distributed·in·the·hope·that·it·will·be·useful,\n"
        moodle.Files.BoilerplateComment:141 replaced token 9 (T_COMMENT on line 10) "//·GNU·General·Public·License·for·more·details.\n" => "//·but·WITHOUT·ANY·WARRANTY;·without·even·the·implied·warranty·of\n"
        moodle.Files.BoilerplateComment:141 replaced token 10 (T_COMMENT on line 11) "//·You·should·have·received·a·copy·of·the·GNU·General·Public·License\n" => "//·MERCHANTABILITY·or·FITNESS·FOR·A·PARTICULAR·PURPOSE.··See·the\n"
        moodle.Files.BoilerplateComment:141 replaced token 11 (T_COMMENT on line 12) "//·along·with·Moodle.··If·not,·see·<https://www.gnu.org/licenses/>.\n" => "//·GNU·General·Public·License·for·more·details.\n"
        * token 11 has already been modified, skipping *
        Squiz.WhiteSpace.SuperfluousWhitespace:220 replaced token 12 (T_COMMENT on line 13) "\r\n" => "\n"
        => Fixing file: 11/2 violations remaining [made 4 passes]...            
        * fixed 11 violations, starting loop 5 *
---START FILE CONTENT---
 1|<?php
 2|// This file is part of Moodle - https://moodle.org/
 3|//
 4|// Moodle is free software: you can redistribute it and/or modify
 5|// it under the terms of the GNU General Public License as published by
 6|// the Free Software Foundation, either version 3 of the License, or
 7|// (at your option) any later version.
 8|//
 9|// Moodle is distributed in the hope that it will be useful,
10|// but WITHOUT ANY WARRANTY; without even the implied warranty of
11|// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
12|// GNU General Public License for more details.
13|
14|
--- END FILE CONTENT ---
        moodle.Files.BoilerplateComment:274 replaced token 11 (T_COMMENT on line 12) "//·GNU·General·Public·License·for·more·details.\n" => "//·GNU·General·Public·License·for·more·details.\n//\n//·You·should·have·received·a·copy·of·the·GNU·General·Public·License\n//·along·with·Moodle.··If·not,·see·<https://www.gnu.org/licenses/>.\n"
        => Fixing file: 1/2 violations remaining [made 5 passes]...             
        * fixed 1 violations, starting loop 6 *
---START FILE CONTENT---
 1|<?php
 2|// This file is part of Moodle - https://moodle.org/
 3|//
 4|// Moodle is free software: you can redistribute it and/or modify
 5|// it under the terms of the GNU General Public License as published by
 6|// the Free Software Foundation, either version 3 of the License, or
 7|// (at your option) any later version.
 8|//
 9|// Moodle is distributed in the hope that it will be useful,
10|// but WITHOUT ANY WARRANTY; without even the implied warranty of
11|// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
12|// GNU General Public License for more details.
13|//
14|// You should have received a copy of the GNU General Public License
15|// along with Moodle.  If not, see <https://www.gnu.org/licenses/>.
16|
17|
--- END FILE CONTENT ---
        => Fixing file: 0/2 violations remaining [made 6 passes]...             
DONE in 152ms
        => File was overwritten
micaherne commented 3 months ago

I'm a bit alarmed that Commenting.InlineComment is removing the blank lines from the boilerplate added by the fix here. I don't understand why this doesn't appear to happen when the file is just the opening PHP tag with an LF after it:

        => Fixing file: 2/2 violations remaining
---START FILE CONTENT---
1|<?php
--- END FILE CONTENT ---
        moodle.Files.BoilerplateComment:241 replaced token 0 (T_OPEN_TAG on line 1) "<?php" => "<?php\n//·This·file·is·part·of·Moodle·-·https://moodle.org/\n//\n//·Moodle·is·free·software:·you·can·redistribute·it·and/or·modify\n//·it·under·the·terms·of·the·GNU·General·Public·License·as·published·by\n//·the·Free·Software·Foundation,·either·version·3·of·the·License,·or\n//·(at·your·option)·any·later·version.\n//\n//·Moodle·is·distributed·in·the·hope·that·it·will·be·useful,\n//·but·WITHOUT·ANY·WARRANTY;·without·even·the·implied·warranty·of\n//·MERCHANTABILITY·or·FITNESS·FOR·A·PARTICULAR·PURPOSE.··See·the\n//·GNU·General·Public·License·for·more·details.\n//\n//·You·should·have·received·a·copy·of·the·GNU·General·Public·License\n//·along·with·Moodle.··If·not,·see·<https://www.gnu.org/licenses/>.\n"
        * token 0 has already been modified, skipping *
        => Fixing file: 1/2 violations remaining [made 1 pass]...               
        * fixed 1 violations, starting loop 2 *
---START FILE CONTENT---
 1|<?php
 2|// This file is part of Moodle - https://moodle.org/
 3|//
 4|// Moodle is free software: you can redistribute it and/or modify
 5|// it under the terms of the GNU General Public License as published by
 6|// the Free Software Foundation, either version 3 of the License, or
 7|// (at your option) any later version.
 8|//
 9|// Moodle is distributed in the hope that it will be useful,
10|// but WITHOUT ANY WARRANTY; without even the implied warranty of
11|// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
12|// GNU General Public License for more details.
13|//
14|// You should have received a copy of the GNU General Public License
15|// along with Moodle.  If not, see <https://www.gnu.org/licenses/>.
16|
--- END FILE CONTENT ---
        => Fixing file: 0/2 violations remaining [made 2 passes]...             
DONE in 29ms
        => File was overwritten

I also don't quite understand why this is not ending up in an infinite loop, where the blank lines are removed then fixed again etc.

Is Commenting.InlineComment supposed to do this or could it be a bug?