PHPDocs that contain /* are not folded

[mikehaertl]

Here's an example:

    /**
     * @param string $path a path like /path/*.csv
     */
    public function a($path)
    {
    }

    /**
     * Process paths like /path/*.csv
     */
    public function b($path)
    {
    }

Will be folded like:

    /**                                                                                                                                           
     * @param string $path a path like /path/*.csv                                                                                                
     */                                                                                                                                           
+--  4 lines: public function a($path)············································································································
    /**                                                                                                                                           
     * Process paths like /path/*.csv                                                                                                             
     */                                                                                                                                           
+--  3 lines: public function b($path)············································································································

[mikehaertl]

@rayburgemeestre Before I try to work on a fix I want to share some thoughts. I would totally understand if you're no longer much acquainted with or interested in that plugin. Just let me know. I just want to make sure that I don't accidentally break things.

I'm also by no means an expert on vimscript so bare with me:

• The script makes a lot of use of global script vars (s:...) to keep it's current state across different function calls. I don't know if that's the way to do things in Vimscript but it definitely doesn't help to make the code easy to follow. (The only way to fix this would probably be a complete rewrite, though.)

• To fix the issue I wonder if this whole block could not be much more simplified if we just continue the loop until we find a line that starts with either /* or /**. That should be much easier than all these substring matches. https://github.com/rayburgemeestre/phpfolding.vim/blob/c7934e0e1a8c96fddc52d34a4d704ded222d8e87/ftplugin/php/phpfolding.vim#L452-L468

• IMO the following block does not only skip empty lines, it will also skip any non-empty line in between empty-lines. Not sure if that's ideal (and not sure about that feature at all). https://github.com/rayburgemeestre/phpfolding.vim/blob/c7934e0e1a8c96fddc52d34a4d704ded222d8e87/ftplugin/php/phpfolding.vim#L436-L445

• The following pattern is used all over the place to match a full line. Why not simply if (match(line, '^\s*$')) instead?https://github.com/rayburgemeestre/phpfolding.vim/blob/c7934e0e1a8c96fddc52d34a4d704ded222d8e87/ftplugin/php/phpfolding.vim#L441

• Here another variable checkLine = s:currentLine is introduced even though s:currentLine is not even used below this line. So there's no need for a new var. https://github.com/rayburgemeestre/phpfolding.vim/blob/c7934e0e1a8c96fddc52d34a4d704ded222d8e87/ftplugin/php/phpfolding.vim#L448

[rayburgemeestre]

Hi @mikehaertl This weekend I have almost no behind the keyboard time, but I will have a look at this issue hopefully tonight or otherwise Monday for sure. I'm no expert by any means w/r/t vimscript, I still use phpfolding but not as often these days. So I think it's great if we can further improve it. My quick impression from briefly looking is you're probably right with your suggestions.

[rayburgemeestre]

It took me a little longer, twisted my back on Monday so wasn't able to do much :grimacing:

• Agreed, it's probably a good idea to get rid of some of the s: globals/constants. It would make things more readable
• Sounds good, but it might break this edge case? https://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_phpDocumentor.howto.pkg.html#basics.docblocktemplate
• I think you're correct that it keeps looking, at least for a max. s:searchPhpDocLineCount lines above, but unless it's just an 'empty' (or containing optional whitespace) line it will not touch s:currentLine
• We can indeed use match() sounds like a better alternative, no idea why it was the way it was (maybe this was how to do it with vim version 6, but we shouldn't care about that anyway)
• I see that checkLine is incremented though, on line 469 though, I dunno if we'd modified s:lineStart if that would mess up some state.

[rayburgemeestre]

P.S. thanks for looking into this plugin. I hope soon to have some time to have a better look at the code myself.

I'm going to think about how we might be able to create some simple unit testing around the script in order to also make refactoring easier. I was thinking a "unit" test could be:

• input some PHP file
• run vim with phpfolding over that code
• somehow get the resulted folds (as actual text probably) and assert whether that matches the expected output

[mikehaertl]

I will also need some time to get back to this issue. Just one thing: How about adding a small demo.php to the repo that contains all (or at least most) common fold cases that this lib covers? Then we can compare before/after our changes to see if things still work as expected. As alternative to a real unit test ...

PS: Just before I wanted to submit my comment I see you added another comment and had the same idea about unit tests :). I think for now a manual test with a demo file would be a good start.

[rayburgemeestre]

I agree with that! Such a demo.php is definitely needed.

w/r/t that unit test automating, I think I actually already found something that we could also automate it with on https://stackoverflow.com/questions/30583388/how-can-i-save-in-vim-a-file-with-the-actual-fold-text-43-lines

(Manually tried it, and it seems to work (:TOhtml /tmp/a.html, then w3m -dump /tmp/a.html))

A quick try of that /**#@+ edge case just now seems to suggest to me actually it is not handled correctly now either. (Or not the way I remember it should work)

/**
 * My Widget class
 */
class Widget
{
    // beginning of docblock template area
    /**#@+
     * @access private
     * @var string
     */
    var $_var1 = 'hello';
    var $_var2 = 'my';
    /**
     * Two words
     */
    var $_var8 = 'like strings';
    /**#@-*/
    var $publicvar = 'Lookee me!';

    public function foo() {
    }
    public static function bar() {

    }
    static public function baz() {

    }
}

I remember having added support for this #@+ stuff working on a very old code base (zend framework version 1) I don't know if anybody still uses this syntax

[rayburgemeestre]

That var $publicvar is getting folded with that #@-

/**#@-*/
var $publicvar = 'Lookee me!';

While I seem to remember that the idea was that everything between @#+ and @#- should be considered one nested block, or just has to be ignored.

[mikehaertl]

I have to admit that I had never seen #@+ before digging through the plugin code and hat to look it up. It's still valid though, according to this: https://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_phpDocumentor.howto.pkg.html#basics.docblocktemplate

But as this issue here is actually about another problem, how about a new issue dedicated to testing?

[mikehaertl]

Ok I found some time to come up with a simple demo.php (see MR) where I tried include examples for all "normal" use cases.

I intentionally left out the following:

• Demo for /**#@-* style templates. To be honest I don't know if folding makes sense for these at all. (And to be very honest: IMO this should not exist in phpDoc at all - phpDoc should not be used as yet another template engine. Stuff like this probably also gives intelephense a hard time).
• Demo for ^\s*\$GLOBALS.*array\s*(. Comment says some PEAR packages use these. But this looks like an arbitrary optimization for some very specific code. I'd say this is also no longer needed.
• Demos for different combinations of plugin config vars

As comments with // are not folded I've used them to mark the expected behavior with // [FOLDED] and // [NOT FOLDED].

Let me know what you think and if something is missing.

Also what do you think about dropping support for #@- and these $GLOBALS folds? If that's OK I could try to cleanup the code a little and also work on the fix for the actual issue here.

[rayburgemeestre]

I also missed your last comment here it seems. Yes, I totally support dropping the #@- and $GLOBALS stuff! Thanks for the contribution. I've merged the demo.php file.

[mikehaertl]

No worries. I see if I find some time over the next days to adress the above topics and also fix this issue here.