Add FopenFlagOrderFixer & FopenFlagsFixer #3812

SpacePossum · 2018-06-05T09:13:26Z

ping @ntzm , hope you can help me out with:

define the order of all flags
~~do we want to have an option to remove flags when found, like t~~
~~do we want to have an option to add flags when not found, like b~~

keradus · 2018-06-07T22:12:01Z

do we want to have an option to remove flags when found, like t
do we want to have an option to add flags when not found, like b

That could be different fixer that would ensure there is always b and never t.
Splitting it into 2 options doesn't make sense to me - from manual:

For portability, it is strongly recommended that you always use the 'b' flag when opening files

SpacePossum · 2018-06-22T11:10:19Z

ping @ntzm could you provide a string that shows all possible flags in the correct order? Than this fixer can be finished :)

ntzm · 2018-06-22T11:24:03Z

Sorry missed your initial ping, will sort at the weekend

ntzm · 2018-06-25T17:14:27Z

b should always be last (apart from +, e.g. wb, wb+), but the order of the others doesn't matter. Might be worth putting them in alphabetical order for uniformity.

SpacePossum · 2018-06-25T21:43:14Z

thanks @ntzm :)
I think this one should be about finished than, let me know if anything is still open

SpacePossum · 2018-07-10T17:21:44Z

@ntzm anything you would like me to change here?

ntzm · 2018-07-10T20:26:21Z

src/Fixer/FunctionNotation/FopenFlagOrderFixer.php

+        $content = $tokens[$argumentFlagIndex]->getContent();
+        $contentQuote = $content[0]; // `'` or `"`
+
+        $content = substr($content, 1, -1);


How about binary strings? e.g. b'rw', B"rw" 😄

ntzm · 2018-07-10T20:39:40Z

src/Fixer/FunctionNotation/FopenFlagOrderFixer.php

+        $argumentFlagIndex = null;
+
+        for ($i = $argumentStartIndex; $i <= $argumentEndIndex; ++$i) {
+            if ($tokens[$i]->equalsAny([[T_WHITESPACE], [T_COMMENT], [T_DOC_COMMENT]])) {


I think isGivenKind([T_WHITESPACE, T_COMMENT, T_DOC_COMMENT]) may be more optimal here

ntzm · 2018-07-10T20:44:18Z

Think something is wrong with my GitHub notifications, seem to be missing some, sorry about the delay

dmvdbrugge

Just minor points

dmvdbrugge · 2018-08-09T10:45:27Z

src/AbstractFopenFlagOrderFixer.php

+ *
+ * @author SpacePossum
+ */
+abstract class AbstractFopenFlagOrderFixer extends AbstractFunctionReferenceFixer


I would suggest removing Order from this name, as only one of the extending classes actually has to do with ordering.

dmvdbrugge · 2018-08-09T10:46:22Z

src/RuleSet.php

@@ -166,6 +166,7 @@
            'dir_constant' => true,
            'ereg_to_preg' => true,
            'error_suppression' => true,
+            'fopen_flag_order' => true,


Do we want the other rule as well?

ntzm · 2018-08-09T11:19:38Z

src/Fixer/FunctionNotation/FopenFlagsFixer.php

+
+        if (strlen($mode) > 13) { // 13 === length 'r+w+a+x+c+etb'
+            return; // sanity check to be less risky
+        }


Line 65 - 79 seem to be duplicated on both fixers, can they be a protected function on the base class?

line 77 shouldn't be the same, besides that I think the shared logic shouldn't be made abstract as it will limit other possible fixers

As the abstract fixer is called AbstractFopenFlagFixer all extensions should operate only on the flags of fopen. I would thus like to make the case that actually all logic in this function except lines 77-86 should be in the parent class. The method now only needs to take in string $mode and return either a fixed or non-altered $mode. For the other fixer, the lines are 78-83 (plus the implode of 84).

I disagree; other fixers might want to operate on more tokens, like complex arguments, and not (only) when the mode is set as literal.

dmvdbrugge · 2018-08-14T07:48:05Z

src/AbstractFopenFlagFixer.php

+            $arguments = $argumentsAnalyzer->getArguments(
+                $tokens,
+                $index,
+                $tokens->findBlockEnd(Tokens::BLOCK_TYPE_PARENTHESIS_BRACE, $index)


This is already done in $this->find() and available as $candidate[2].

nice catch, fixed

keradus · 2018-08-22T18:57:25Z

Thank you @SpacePossum.

@ntzm

This PR was merged into the 2.13-dev branch. Discussion ---------- Add FopenFlagOrderFixer & FopenFlagsFixer closes #3417 ping @ntzm , hope you can help me out with: - define the order of all flags - ~do we want to have an option to remove flags when found, like `t`~ - ~do we want to have an option to add flags when not found, like `b`~ Commits ------- 8682d86 Add FopenFlagOrderFixer & FopenFlagsFixer

SpacePossum added this to the 2.13.0 milestone Jun 5, 2018

SpacePossum added kind/feature WIP labels Jun 6, 2018

SpacePossum added the status/input required For the issue to be confirmed or the PR to be process; additional input of the author is required label Jun 22, 2018

SpacePossum removed WIP status/input required For the issue to be confirmed or the PR to be process; additional input of the author is required labels Jun 25, 2018

ntzm suggested changes Jul 10, 2018

View reviewed changes

SpacePossum added the WIP label Jul 11, 2018

SpacePossum changed the title ~~FopenFlagOrderFixer - introduction~~ FopenFlagOrderFixer & FopenFlagsFixer - introduction Aug 8, 2018

SpacePossum removed the WIP label Aug 8, 2018

dmvdbrugge reviewed Aug 9, 2018

View reviewed changes

ntzm suggested changes Aug 9, 2018

View reviewed changes

dmvdbrugge suggested changes Aug 14, 2018

View reviewed changes

dmvdbrugge approved these changes Aug 16, 2018

View reviewed changes

keradus added has PR and removed has PR labels Aug 16, 2018

keradus changed the title ~~FopenFlagOrderFixer & FopenFlagsFixer - introduction~~ Add FopenFlagOrderFixer & FopenFlagsFixer Aug 22, 2018

keradus approved these changes Aug 22, 2018

View reviewed changes

keradus added the RTM Ready To Merge label Aug 22, 2018

Add FopenFlagOrderFixer & FopenFlagsFixer

8682d86

keradus removed the RTM Ready To Merge label Aug 22, 2018

keradus merged commit 8682d86 into PHP-CS-Fixer:master Aug 22, 2018

keradus deleted the master_FopenFlagOrderFixer branch August 22, 2018 18:57

nicolas-grekas mentioned this pull request Oct 2, 2018

Allow removing the "b" mode from fopen calls #4011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FopenFlagOrderFixer & FopenFlagsFixer #3812

Add FopenFlagOrderFixer & FopenFlagsFixer #3812

SpacePossum commented Jun 5, 2018 •

edited

keradus commented Jun 7, 2018

SpacePossum commented Jun 22, 2018

ntzm commented Jun 22, 2018

ntzm commented Jun 25, 2018

SpacePossum commented Jun 25, 2018

SpacePossum commented Jul 10, 2018

ntzm Jul 10, 2018

ntzm Jul 10, 2018

ntzm commented Jul 10, 2018

dmvdbrugge left a comment

dmvdbrugge Aug 9, 2018

dmvdbrugge Aug 9, 2018

ntzm Aug 9, 2018

SpacePossum Aug 9, 2018 •

edited

dmvdbrugge Aug 13, 2018

SpacePossum Aug 14, 2018

dmvdbrugge Aug 14, 2018

SpacePossum Aug 15, 2018

keradus commented Aug 22, 2018

Add FopenFlagOrderFixer & FopenFlagsFixer #3812

Add FopenFlagOrderFixer & FopenFlagsFixer #3812

Conversation

SpacePossum commented Jun 5, 2018 • edited

keradus commented Jun 7, 2018

SpacePossum commented Jun 22, 2018

ntzm commented Jun 22, 2018

ntzm commented Jun 25, 2018

SpacePossum commented Jun 25, 2018

SpacePossum commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntzm commented Jul 10, 2018

dmvdbrugge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpacePossum Aug 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keradus commented Aug 22, 2018

SpacePossum commented Jun 5, 2018 •

edited

SpacePossum Aug 9, 2018 •

edited