-
Notifications
You must be signed in to change notification settings - Fork 11.7k
Refactor View package #7961
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
Refactor View package #7961
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crynobone PHP5.4+ has <?= short tag enabled by default. And Laravel requires 5.4+, so why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And everyone is forced to have short_open_tag as on?
I still don't see it really add any benefit, unless this changes gain some huge performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're wrong. <?= is not even considered as a short tag anymore, and is not affected by short_open_tag. See http://svn.php.net/viewvc?view=revision&revision=311260, from Rasmus Lerdorf himself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't see it really add any benefit, unless this changes gain some huge performance.
Well, just as [] is not of any more benefits than array(). But Laravel has always been a modern framework, it should adapt the latest techniques whenever it can. I see quite a portion of L5 switched to [] instead of array(), ?: instead of ? $var : $val, so again, why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the use of non-conventional tags.
|
The problems with <? is one of convention and good practise - it's a sharp conflict with XML, which is why it is generally frowned upon to be used as a language opening tag. I cannot see this being supported by @taylorotwell or anyone in the PHP community in general. |
|
First, I wouldn't speak for the whole PHP community, which I'm pretty much sure I'm a part of. Then, would appreciate if you could enlighten me regarding this statement:
And last, it's perfectly OK if Taylor or any contributor rejects this PR. I still strongly believe that Laravel has much for refactoring. |
|
That's why i said "in general" - aka, a generalisation. Aka, the majority. Have a look at the tags xml uses. Tell me if this looks familiar: <?xml version="1.0" encoding="UTF-8"?>In your code, if someone is using the View package to generate XML - it will fail. Their XML documents won't be correctly constructed AND any opening XML tag will be incorrectly parsed as PHP. |
|
"Generalization" is a good fallacy name. Not sure what you mean by comparing Oh, and for that, I believe Rasmus is in a better position to represent the "PHP community." |
|
Please read: http://php.net/manual/en/language.basic-syntax.phptags.php "PHP also allows for short open tag <? (which is discouraged since it is only available if enabled using the short_open_tag php.ini configuration file directive, or if PHP was configured with the --enable-short-tags option)." If you support short tags, then XML tags also get factored in. If you're going to support short tags, and allow for <?= then you also have to support <? which creates problems, as I mentioned above - with XML. If short tags are actually discouraged on the OFFICIAL documentation - then we shouldn't be supporting them. |
|
@kirkbushell, I strongly suggest you read from the beginning of this discussion. |
|
No it's okay, I'm done. |
|
This add no benefit. All you're done is changed the syntax. We don't accept pulls for short array syntax, and there's no point in chaining the tags to a style that might not even work on all systems. |
|
@GrahamCampbell While I totally respect your decision, the PR had more than just array syntax. Even if it was only array syntax, I wouldn't say an effort to make the code cleaner is of "no benefit."
Again, how does
Maybe @taylorotwell could clarify this? No hard feelings, only wanted to learn things. |
Just because it's "enabled by default", it doesn't mean it's always enabled. Also, those tags might be removed in php 7 are far as I know. |
|
@GrahamCampbell Stop spreading nonsense. @phanan has already given proof that |
|
Just read up on PHP 5.4, absolutely correct - my bad. They are enabled - and you cannot disable them. |
|
@GrahamCampbell As some guys here have pointed out as well, |
|
Yes, please reopen this. PSR-1:
and
It's completely valid and follows PSR-1. |
|
@phanan your passive-aggressive responses need to stop - they're unnecessary. |
|
@kirkbushell Sorry, didn't realize I was being passive-aggressive. And sorry for bad English. 😃 |
I've made some refactoring for the whole View package.