-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
PSR12 standard #750
Comments
I've been following it, but I haven't got time to do anything about it. |
There are a few points that will hardly change in the future and can most certainly be added as sniffs
Do you think is a good idea to create a PSR12 standard before it is established (starting with this points) and then go updating it, or wait until it is fully finished? |
Most of the hard work is done, but some custom sniffs will need to be written. It should be possible for anyone to create a ruleset.xml file that checks almost all of PSR12 today. But I don't think I'd include it in PHPCS until it's done. |
Closing this. I'll re-evaluate if the PSR-12 fighting ever stops and something gets released. |
@gsherwood |
Now is in review https://www.php-fig.org/psr/#review . Anyone have some tips am how to enforce those recommendations until something official is released? |
@gsherwood Maybe its time to re-open this issue as I suspect more questions will start coming about this ? |
As far as i'Ve seen the FIG is looking for a second example implementation. Wouldn't a sniff for codesniffer be a good way to prove the concept? Or would an external sniff be more of interest? |
@heiglandreas What does an "example implementation" constitute though ? |
The current Example-Implementation is https://github.com/KorvinSzanto/PHP-CS-Fixer/tree/feature/psr12 and from what I can read from it it's an implementation that enforces PRS-12 on a codebase. The same thing So AFAIK it's not about implementing PSR-12 inside the sniff but to create a sniff that finds violations of PSR-12. |
@heiglandreas I've got a few sniffs which cover parts of PSR-12 ready already and I seem to recall @timoschinkel has a large part of PSR-12 covered already as well. @gsherwood Would now be a good time maybe to start with a PSR-12 ruleset and allow people who have sniffs covering parts of PSR-12 which aren't already covered by sniffs in PHPCS, to start pulling them ? Want me to have a go at making an initial ruleset with what can be covered by existing sniffs ? |
Yes, I think so. |
Sorry, missed this. If you'd like to :) If you could comment it like I did for PSR2, it helps me maintain it going forward and makes sure we've covered everything: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/PSR2/ruleset.xml |
@gsherwood I'll try and get to it later this week. In the mean time, you may want to reopen this issue ? |
I've committed a ruleset for PSR-12, which pulls in a few current sniffs and includes a couple of new ones. Lots of work to go though. |
Inspiration from PHP CS Fixer PSR-12 set: https://github.com/Symplify/Symplify/blob/master/packages/EasyCodingStandard/config/psr12.yml |
For my own reference:
|
I know the ruleset is still being worked, but I see the following issue: $foo .= $bar; Generates the following (incorrectly, I presume, as
|
@tag That code does not generate an error for me. Are you sure you don't have a syntax error in your file causing incorrect tokenizing? Can you share a code sample that replicates the error alone? |
@tag I just tried to reproduce the issue and that code snippet does not produce any errors for me against the |
… spacing and indents are correct inside control structure parenthesis (ref #750)
… of classes/interfaces/traits/functions are not followed by a comment or statement (ref #750) Fixers were not added to this sniff as it is likely that comments would be found more than anything else, and simply moving them to the next line is probably not the right fix. More likely, the comment should be removed, which only the developer should do.
Another update: Only the checks for anon classes need to be done now. I think everything else has been taken care of. First thing to do is figure out what "Anonymous Classes MUST follow the same guidelines and principles as closures in the above section" really means, then I'll add a sniff for it. |
These are the anon class rules that I believe PSR-12 wants enforced:
|
Do you have an idear when there will be a new release of phpcs with the full psr12 rules ? Thank you. |
I've got a very busy week this week, so I might not complete the PSR-12 standard until next week. That would mean a release should be ready in the next 2 weeks. |
great news. keep up the good work. |
I've committed the anon class sniff, which makes heavy use of the main class declaration sniff, as well as rules from function declarations and closures for when arguments are passed into the constructor. (I forgot to mention this issue on the commit, but it is here: b770ed3) So that means a basic PSR12 standard is now complete in PHPCS. I'll leave this issue open in case anyone can provide some testing and feedback. I'll work on the remaining few 3.5.0 issues and then do a release either this week or next, depending on the feedback I get. |
Hello, I tested against my project. I have a symfony project which has a file like this:
I get the following two errors:
does not feel right. |
Furthermore on a file like this:
It does not add a blank line between Not sure if it is one of the things you left for later. |
Yeah, it's hard to tell if that docblock is a file-level docblock or a class docblock. What you've actually got is no file-level docblock, but it looks to the sniff like you've just put it in the wrong spot. I think the solution is to do what some others sniffs do, which is to just assume the docblocks followed by a class/interface/trait just belong to that OO structure.
It does for me:
What does it show you? Also, thanks a lot for testing this out. I really appreciate it. |
Code wasn't taking prefixes into account when determining if the docblock belong to a scope opener
I actually already had code in there for the, but the class and method prefixes were not being taken into account. I've fixed that, and the sample code now produces no errors. |
Thanks @gsherwood for all the hard work you've put in and continue to do so Everything looks good apart from one aspect of PSR-12 which interferes with Laravel Blade view files. However, I'm not sure anything can be done. A typical Blade file might start with the following: @extends('layouts.app')
@section('comments')
<?php
/**
*
*
* @author
* @version
* @copyright
*/
?>
@endsection This will lead to the error: One solution would be to replace the |
I am wondering why you would want to have a licence header in that place though. It seems like it being the first thing in the file would make more sense I guess? |
@spaceemotion here is another example. It's less about the contents of the PHP and more about using Laravel Blade syntax ( @extends('layouts.app')
@section('imports')
<?php
use App\Models\User;
?>
@endsection |
I'm not sure what can be done either. PSR-12 probably didn't consider template files when the standard was written, and the header part of the standard is very clear. It's a pain, but you could exclude that specific sniff from your template files using a custom ruleset, along with a path or regex that matches the template file names or extensions. Something like: <rule ref="PSR12.Files.FileHeader">
<exclude-pattern>*/templates/*</exclude-pattern>
</rule> I'm not really sure beyond that. |
I personally believe that the can be treated as if the use statement in this case is part of the "rest of the code" part, as the use statement is not nessecarily part of the file.
The equivalent of the original in full blade syntax would be
or
or
depending on which version you are using. |
Hello.. not sure if I did something wrong before but yes it works...
Yes.. checked and it works... 👍 |
This code passes PSR12, except for a few blank lines that are needed in the header section: <?php
/**
* File Based Doc block etc...
*/
?>
@extends('layouts.app')
@section('imports')
<?php
// rest of code
use App\Models\User;
?>
@endsection |
Marking as complete in preparation for a release. Any bugs in the PSR12 implementation should now be opened as seperate issues. Thanks to everyone who participated here. |
I'm going to have to remove PSR12 sniffing, because apparently |
You mentioned something similar on another issue as well.. TBH I can not understand what's the exact problem you are trying to solve.. |
@hopeseekr disable You could also raise an issue on the PHP-Fig mailing list about how you disagree with #3 on PSR-12, namely:
|
We'll match the PHP minimum requirement exactly to Monolog 1, which is 5.3.0. The unit tests require 5.3.3 (because PHPUnit 4.8 requires 5.3.3), but technically the runtime requirement is just 5.3.0. We'll also require PHP_CodeSniffer 3.5 or later specifically, as we need the PSR-12 definition, and that was [added in 3.5.0](squizlabs/PHP_CodeSniffer#750).
I know this might be a bit too early as the Extended Coding Style Guide is still on Draft phase but IMO having early access to what could be the standard in the end could be helpful
What anyone else think about this?
The text was updated successfully, but these errors were encountered: