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

[5.0] PSR-2 AND PSR-5 #6941

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@GrahamCampbell
Member

GrahamCampbell commented Jan 7, 2015

No description provided.

@Anahkiasen

This comment has been minimized.

Show comment
Hide comment
@Anahkiasen

Anahkiasen Jan 7, 2015

Contributor

👍

Contributor

Anahkiasen commented Jan 7, 2015

👍

@GrahamCampbell GrahamCampbell changed the title from [5.0] [WIP] PSR-2 AND PSR-5 to [5.0] PSR-2 AND PSR-5 Jan 7, 2015

@RomainLanz

This comment has been minimized.

Show comment
Hide comment
@RomainLanz

RomainLanz Jan 7, 2015

Contributor

👍

What do you think about keep the namespace on the same line that the <?php tag?

Contributor

RomainLanz commented Jan 7, 2015

👍

What do you think about keep the namespace on the same line that the <?php tag?

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 7, 2015

Member

PSR-2 doesn't allow it unfortunately.

Member

GrahamCampbell commented Jan 7, 2015

PSR-2 doesn't allow it unfortunately.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 7, 2015

Member

It says it must be on a new line, and common convention is to leave another line between that opening tag and the namespace, so I did that.

Member

GrahamCampbell commented Jan 7, 2015

It says it must be on a new line, and common convention is to leave another line between that opening tag and the namespace, so I did that.

@RomainLanz

This comment has been minimized.

Show comment
Hide comment
@RomainLanz

RomainLanz Jan 7, 2015

Contributor

I never see that is not possible for the namespace. The only one rule for the position is : When present, there MUST be one blank line after the namespace declaration.

When present, there MUST be one blank line after the namespace declaration.

When present, all use declarations MUST go after the namespace declaration.

There MUST be one use keyword per declaration.

There MUST be one blank line after the use block.
Contributor

RomainLanz commented Jan 7, 2015

I never see that is not possible for the namespace. The only one rule for the position is : When present, there MUST be one blank line after the namespace declaration.

When present, there MUST be one blank line after the namespace declaration.

When present, all use declarations MUST go after the namespace declaration.

There MUST be one use keyword per declaration.

There MUST be one blank line after the use block.
@Anahkiasen

This comment has been minimized.

Show comment
Hide comment
@Anahkiasen

Anahkiasen Jan 7, 2015

Contributor

It's not in PSR2 but to be fair apart from in Laravel I've never seen it on the same line and since the point here is to go towards unification with other frameworks, I'd say putting it on a new line would be best no?

Contributor

Anahkiasen commented Jan 7, 2015

It's not in PSR2 but to be fair apart from in Laravel I've never seen it on the same line and since the point here is to go towards unification with other frameworks, I'd say putting it on a new line would be best no?

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 7, 2015

Member

Agreed.

Member

GrahamCampbell commented Jan 7, 2015

Agreed.

@JoostK

This comment has been minimized.

Show comment
Hide comment
@JoostK

JoostK Jan 7, 2015

Contributor

Why does PSR-2 have to be so ugly 😢

Contributor

JoostK commented Jan 7, 2015

Why does PSR-2 have to be so ugly 😢

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 7, 2015

Member

PSR-2 is pretty :) I've got used to it.

Member

GrahamCampbell commented Jan 7, 2015

PSR-2 is pretty :) I've got used to it.

@RomainLanz

This comment has been minimized.

Show comment
Hide comment
@RomainLanz

RomainLanz Jan 7, 2015

Contributor

PSR-2 is not ugly IMO.

Ok, no problem for the namespace, I just think that it's better to have it on the same line.

Contributor

RomainLanz commented Jan 7, 2015

PSR-2 is not ugly IMO.

Ok, no problem for the namespace, I just think that it's better to have it on the same line.

@hannesvdvreken

This comment has been minimized.

Show comment
Hide comment
@hannesvdvreken

hannesvdvreken Jan 7, 2015

Contributor

👍

Contributor

hannesvdvreken commented Jan 7, 2015

👍

@KennedyTedesco

This comment has been minimized.

Show comment
Hide comment
@KennedyTedesco

KennedyTedesco Jan 8, 2015

Contributor

Beautiful! 👍

Contributor

KennedyTedesco commented Jan 8, 2015

Beautiful! 👍

@shin1x1

This comment has been minimized.

Show comment
Hide comment
@shin1x1

shin1x1 commented Jan 8, 2015

👍

@tomschlick

This comment has been minimized.

Show comment
Hide comment
@tomschlick

tomschlick Jan 8, 2015

Contributor

👍

Contributor

tomschlick commented Jan 8, 2015

👍

*/
public function retrieveByToken($identifier, $token)
{
$user = $this->conn->table($this->table)
->where('id', $identifier)
->where('remember_token', $token)
->first();

This comment has been minimized.

@ibrasho

ibrasho Jan 8, 2015

Contributor

Alignment?

@ibrasho

ibrasho Jan 8, 2015

Contributor

Alignment?

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Jan 8, 2015

Member

I have no automated way to fix such cases. Also, I don't think PSR-2 says anything about this.

@GrahamCampbell

GrahamCampbell Jan 8, 2015

Member

I have no automated way to fix such cases. Also, I don't think PSR-2 says anything about this.

This comment has been minimized.

@ibrasho

ibrasho Jan 8, 2015

Contributor

I know, but I think we want to keep the Laravel-ish feel. :-)

@ibrasho

ibrasho Jan 8, 2015

Contributor

I know, but I think we want to keep the Laravel-ish feel. :-)

*/
public function updateRememberToken(UserContract $user, $token)
{
$this->conn->table($this->table)

This comment has been minimized.

@ibrasho

ibrasho Jan 8, 2015

Contributor

Alignment?

@ibrasho

ibrasho Jan 8, 2015

Contributor

Alignment?

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Jan 8, 2015

Member

the alignment is correct here

@GrahamCampbell

GrahamCampbell Jan 8, 2015

Member

the alignment is correct here

*/
protected function attemptBasic(Request $request, $field)
{
if (! $request->getUser()) {

This comment has been minimized.

@ibrasho

ibrasho Jan 8, 2015

Contributor

Should not this be if ( ! $request->getUser()) {?

@ibrasho

ibrasho Jan 8, 2015

Contributor

Should not this be if ( ! $request->getUser()) {?

This comment has been minimized.

@RomainLanz

RomainLanz Jan 8, 2015

Contributor

The exclamation point is not reglemented on PSR-2, I think it's more readable to keep a space before and after it too.

What do you think @GrahamCampbell ?

@RomainLanz

RomainLanz Jan 8, 2015

Contributor

The exclamation point is not reglemented on PSR-2, I think it's more readable to keep a space before and after it too.

What do you think @GrahamCampbell ?

This comment has been minimized.

@crynobone

crynobone Jan 8, 2015

Contributor

It's part of PSR-2

Opening parentheses for control structures MUST NOT have a space after them, and closing parentheses for control structures MUST NOT have a space before.

@crynobone

crynobone Jan 8, 2015

Contributor

It's part of PSR-2

Opening parentheses for control structures MUST NOT have a space after them, and closing parentheses for control structures MUST NOT have a space before.

This comment has been minimized.

@crynobone

crynobone Jan 8, 2015

Contributor
if ( $foo ) { // wrong
if ($foo) { // right
if (! $foo) { // is not wrong :P
@crynobone

crynobone Jan 8, 2015

Contributor
if ( $foo ) { // wrong
if ($foo) { // right
if (! $foo) { // is not wrong :P

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Jan 8, 2015

Member

Yeh, this is fine as it is.

@GrahamCampbell

GrahamCampbell Jan 8, 2015

Member

Yeh, this is fine as it is.

This comment has been minimized.

@gaomd

gaomd Jan 9, 2015

Contributor

if ( ! $foo) { // seems more readable

@gaomd

gaomd Jan 9, 2015

Contributor

if ( ! $foo) { // seems more readable

This comment has been minimized.

@gregrobson

gregrobson Feb 4, 2015

Personally I have found that removing the negation from the if statement is the best solution as it improves readability.

if (! $request->getUser()) {
// vs
if ($request->noUserSpecified()) {

I haven't used the above method in my own code, but hopefully it demonstrates the idea. as another example it's far easier to say is empty than does not have more than 0 items

@gregrobson

gregrobson Feb 4, 2015

Personally I have found that removing the negation from the if statement is the best solution as it improves readability.

if (! $request->getUser()) {
// vs
if ($request->noUserSpecified()) {

I haven't used the above method in my own code, but hopefully it demonstrates the idea. as another example it's far easier to say is empty than does not have more than 0 items

@anhskohbo

This comment has been minimized.

Show comment
Hide comment
@anhskohbo

anhskohbo Jan 8, 2015

Contributor

👍

Contributor

anhskohbo commented Jan 8, 2015

👍

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jan 8, 2015

Member

Rebase? :)

Member

taylorotwell commented Jan 8, 2015

Rebase? :)

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 8, 2015

Member

Will do when I find a min.

Member

GrahamCampbell commented Jan 8, 2015

Will do when I find a min.

@hannesvdvreken

This comment has been minimized.

Show comment
Hide comment
@hannesvdvreken

hannesvdvreken Jan 8, 2015

Contributor

Cool! 👍

Contributor

hannesvdvreken commented Jan 8, 2015

Cool! 👍

@jwpage

This comment has been minimized.

Show comment
Hide comment
@jwpage

jwpage Jan 9, 2015

Just curious, will there be an accompanying PR for laravel/laravel so that the code there is PSR-2+5 as well, or is this only going to be a standard for laravel/framework?

jwpage commented Jan 9, 2015

Just curious, will there be an accompanying PR for laravel/laravel so that the code there is PSR-2+5 as well, or is this only going to be a standard for laravel/framework?

@xiaobeicn

This comment has been minimized.

Show comment
Hide comment
@xiaobeicn

xiaobeicn commented Jan 9, 2015

👍

@alioygur

This comment has been minimized.

Show comment
Hide comment
@alioygur

alioygur Jan 9, 2015

Contributor

👍

Contributor

alioygur commented Jan 9, 2015

👍

@alioygur

This comment has been minimized.

Show comment
Hide comment
@alioygur

alioygur Jan 9, 2015

Contributor

@GrahamCampbell it should be on laravel/laravel

Contributor

alioygur commented Jan 9, 2015

@GrahamCampbell it should be on laravel/laravel

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 9, 2015

Member

Yeh, I will do it there too. I'll do it once Taylor's totally happy with the default app.

Member

GrahamCampbell commented Jan 9, 2015

Yeh, I will do it there too. I'll do it once Taylor's totally happy with the default app.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 9, 2015

Member

And indeed on the other laravel repos.

Member

GrahamCampbell commented Jan 9, 2015

And indeed on the other laravel repos.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 9, 2015

Member

NB, I'll be rebasing this once 4.2.17 is tagged, and merged with the master branch.

Member

GrahamCampbell commented Jan 9, 2015

NB, I'll be rebasing this once 4.2.17 is tagged, and merged with the master branch.

@zombopanda

This comment has been minimized.

Show comment
Hide comment
@zombopanda

zombopanda commented Jan 11, 2015

👍

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 19, 2015

Member

I'll resend this at a later point when we're truly ready to do this. The merging of this pull will mean that Laravel 4.2 is no-longer maintained for anything but more important bug fixes, so we'll be doing this close to the 5.0.0 release.

Member

GrahamCampbell commented Jan 19, 2015

I'll resend this at a later point when we're truly ready to do this. The merging of this pull will mean that Laravel 4.2 is no-longer maintained for anything but more important bug fixes, so we'll be doing this close to the 5.0.0 release.

@GrahamCampbell GrahamCampbell deleted the 5.0-cs branch Jan 19, 2015

@JosephSilber

This comment has been minimized.

Show comment
Hide comment
@JosephSilber

JosephSilber Jan 20, 2015

Contributor

@GrahamCampbell at that cutoff point, shouldn't we also convert all arrays to the short syntax?

Contributor

JosephSilber commented Jan 20, 2015

@GrahamCampbell at that cutoff point, shouldn't we also convert all arrays to the short syntax?

@ibrasho

This comment has been minimized.

Show comment
Hide comment
@ibrasho

ibrasho Jan 20, 2015

Contributor

^ I agree.

Contributor

ibrasho commented Jan 20, 2015

^ I agree.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 20, 2015

Member

@GrahamCampbell at that cutoff point, shouldn't we also convert all arrays to the short syntax?

I already did that as part of this change, and yeh, I will be doing that in the replacement too.

Member

GrahamCampbell commented Jan 20, 2015

@GrahamCampbell at that cutoff point, shouldn't we also convert all arrays to the short syntax?

I already did that as part of this change, and yeh, I will be doing that in the replacement too.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 20, 2015

Member

NB, only on 5.0.

Member

GrahamCampbell commented Jan 20, 2015

NB, only on 5.0.

@HipsterJazzbo

This comment has been minimized.

Show comment
Hide comment
@HipsterJazzbo

HipsterJazzbo Feb 4, 2015

Contributor

Is this going to happen for release? Can't stand the suspense!

Contributor

HipsterJazzbo commented Feb 4, 2015

Is this going to happen for release? Can't stand the suspense!

@overtrue

This comment has been minimized.

Show comment
Hide comment
@overtrue

overtrue Feb 4, 2015

Contributor

PSR-2 is pretty 👍

Contributor

overtrue commented Feb 4, 2015

PSR-2 is pretty 👍

@daids

This comment has been minimized.

Show comment
Hide comment
@daids

daids commented Feb 4, 2015

👍

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 4, 2015

Member

This is not ever going to happen, sorry.

Member

GrahamCampbell commented Feb 4, 2015

This is not ever going to happen, sorry.

@overtrue

This comment has been minimized.

Show comment
Hide comment
@overtrue

overtrue Feb 4, 2015

Contributor

@GrahamCampbell What a sad ending... 😢

Contributor

overtrue commented Feb 4, 2015

@GrahamCampbell What a sad ending... 😢

@Anahkiasen

This comment has been minimized.

Show comment
Hide comment
@Anahkiasen

Anahkiasen Feb 4, 2015

Contributor

Contributor

Anahkiasen commented Feb 4, 2015

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 4, 2015

Member

I'm a bit gutted too. Taylor does have a point that this whole cs thing is a total waste of time.

The big issue for me is not that laravel/framework's code isn't PSR-2, but all the generated files arn;t PSR-2, which means I have to change them every time. :(

Member

GrahamCampbell commented Feb 4, 2015

I'm a bit gutted too. Taylor does have a point that this whole cs thing is a total waste of time.

The big issue for me is not that laravel/framework's code isn't PSR-2, but all the generated files arn;t PSR-2, which means I have to change them every time. :(

@Anahkiasen

This comment has been minimized.

Show comment
Hide comment
@Anahkiasen

Anahkiasen Feb 4, 2015

Contributor

Taylor does have a point that this whole cs thing is a total waste of time

I don't think that's a good argument, I'd rather have it be a waste of time once and then being able to let a tool fix them every time

Contributor

Anahkiasen commented Feb 4, 2015

Taylor does have a point that this whole cs thing is a total waste of time

I don't think that's a good argument, I'd rather have it be a waste of time once and then being able to let a tool fix them every time

@MaartenStaa

This comment has been minimized.

Show comment
Hide comment
@MaartenStaa

MaartenStaa Feb 4, 2015

I have to agree with Maxime. It's quite a bit of work up front, but once you're there, there's a lot of nice tools for enforcing PSR-2.

It personally took me a while to get used to the standard, but now I think it's a prettier CS than what Laravel uses. Plus, since so many packages/frameworks use it, that's really a big plus IMO.

MaartenStaa commented Feb 4, 2015

I have to agree with Maxime. It's quite a bit of work up front, but once you're there, there's a lot of nice tools for enforcing PSR-2.

It personally took me a while to get used to the standard, but now I think it's a prettier CS than what Laravel uses. Plus, since so many packages/frameworks use it, that's really a big plus IMO.

@crynobone

This comment has been minimized.

Show comment
Hide comment
@crynobone

crynobone Feb 4, 2015

Contributor

I'm a bit gutted too. Taylor does have a point that this whole cs thing is a total waste of time.

It would be useful if Laravel 4.2 still support minor bugfix (instead of just security fix). Expecting everyone to jump to 5 straight away when they have production code when what not available right now in 4.2 is some minor bugs IMHO is not acceptable. The idea right now is Laravel 5 going to be available today (update now as Laravel 4 is EOL unless there some security issue).

p/s: I am under the assumption that 4 become security only due to moving to PSR-2.

Contributor

crynobone commented Feb 4, 2015

I'm a bit gutted too. Taylor does have a point that this whole cs thing is a total waste of time.

It would be useful if Laravel 4.2 still support minor bugfix (instead of just security fix). Expecting everyone to jump to 5 straight away when they have production code when what not available right now in 4.2 is some minor bugs IMHO is not acceptable. The idea right now is Laravel 5 going to be available today (update now as Laravel 4 is EOL unless there some security issue).

p/s: I am under the assumption that 4 become security only due to moving to PSR-2.

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Feb 4, 2015

Contributor

It's a total waste of time having to explain the differences of the Laravel CS to everyone, commenting on PR's, not having to validate with proper tools, no default settings in most IDE's etc.
Regular contributors know the difference, but it's annoying to keep on switching. External contributors usually just assume PSR-2..

If Taylor doesn't care, but a lot of contributors do, why not listen to them?

Contributor

barryvdh commented Feb 4, 2015

It's a total waste of time having to explain the differences of the Laravel CS to everyone, commenting on PR's, not having to validate with proper tools, no default settings in most IDE's etc.
Regular contributors know the difference, but it's annoying to keep on switching. External contributors usually just assume PSR-2..

If Taylor doesn't care, but a lot of contributors do, why not listen to them?

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 4, 2015

Member

I think the point he's making is it doesn't really matter if the cs of contributions is incorrect.

He's made up his mind already anyway. I want everything to go PSR-2, but he doesn't.

Member

GrahamCampbell commented Feb 4, 2015

I think the point he's making is it doesn't really matter if the cs of contributions is incorrect.

He's made up his mind already anyway. I want everything to go PSR-2, but he doesn't.

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Feb 4, 2015

Contributor

I think the point he's making is it doesn't really matter if the cs of contributions is incorrect.

Wait, are you saying we can just submit PR's in any CS we want? So just submitting stuff in PSR-2 is okay too? ;)

Contributor

barryvdh commented Feb 4, 2015

I think the point he's making is it doesn't really matter if the cs of contributions is incorrect.

Wait, are you saying we can just submit PR's in any CS we want? So just submitting stuff in PSR-2 is okay too? ;)

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 4, 2015

Member

No, it's not ok. Hmmm. This is a mess if you ask me. :/

Member

GrahamCampbell commented Feb 4, 2015

No, it's not ok. Hmmm. This is a mess if you ask me. :/

@Garbee

This comment has been minimized.

Show comment
Hide comment
@Garbee

Garbee Feb 4, 2015

Contributor

imo, if a PR is sent with improper CS it would be just as quick to pull the PR in locally, fix the CS and push it up then it would be commenting on every little damn thing. However, that would end up being a huge time resource with all the little issues found then it would be to just go PSR-2 and let tools handle it.

Everyone is correct, this does a major hurt exactly once, aside from needing to backport fixes since it ends up making more work to do that between 5.x and 4.x. But once that is done everything is more seamless for the majority of developers using the platform.

Moving to PSR-2 could also lead to more contributions back since it is less friction to make sure you are coding to the Laravel standard instead of PSR-2 (or any other code style that has a wide array of tooling available.)

However, it looks like 5.0 has gone stable within the past few hours with all the site updates and all. So this shipped has sailed for this major. Maybe with 6.0...

Contributor

Garbee commented Feb 4, 2015

imo, if a PR is sent with improper CS it would be just as quick to pull the PR in locally, fix the CS and push it up then it would be commenting on every little damn thing. However, that would end up being a huge time resource with all the little issues found then it would be to just go PSR-2 and let tools handle it.

Everyone is correct, this does a major hurt exactly once, aside from needing to backport fixes since it ends up making more work to do that between 5.x and 4.x. But once that is done everything is more seamless for the majority of developers using the platform.

Moving to PSR-2 could also lead to more contributions back since it is less friction to make sure you are coding to the Laravel standard instead of PSR-2 (or any other code style that has a wide array of tooling available.)

However, it looks like 5.0 has gone stable within the past few hours with all the site updates and all. So this shipped has sailed for this major. Maybe with 6.0...

@Anahkiasen

This comment has been minimized.

Show comment
Hide comment
@Anahkiasen

Anahkiasen Feb 4, 2015

Contributor

Honestly considering how different the L5 and L4 codebases are now I don't think CS are would even be the main reasons backports are hard

Contributor

Anahkiasen commented Feb 4, 2015

Honestly considering how different the L5 and L4 codebases are now I don't think CS are would even be the main reasons backports are hard

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 4, 2015

Member

If Taylor did change, he'd only do it on the framework repo, and not the other repos, which I also think is very strange. It's to late now anyway.

Member

GrahamCampbell commented Feb 4, 2015

If Taylor did change, he'd only do it on the framework repo, and not the other repos, which I also think is very strange. It's to late now anyway.

@laravel laravel locked and limited conversation to collaborators Feb 4, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.