Switches all appropriate echos to the short echo#1137
Switches all appropriate echos to the short echo#1137alecho wants to merge 3 commits intocakephp:3.0from alecho:feature-convert-to-short-echo
echos to the short echo#1137Conversation
|
By the way, there's a current PR pending for the blog tutorial: #1133 |
|
@bcrowe Good to know. I can rebase after that is in 3.0 |
There was a problem hiding this comment.
While semicolons are not needed at end of statement when using short echo tags, personally I would prefer cake recommending using them.
There was a problem hiding this comment.
@ADmad I agree, but ... https://github.com/cakephp/docs/pull/1136/files 😷
There was a problem hiding this comment.
@bcrowe Let's see if others have an opinion on this. Else we keep it without semicolons. While i would prefer having them wouldn't really mind if we chose to skip them either.
There was a problem hiding this comment.
I think the lack of a semicolon on the single lines, like this one, above look cleaner and feels more like a templating language syntax like handlebars. Besides that I don't really have a preference either way.
Just as a reminder there was some discussion on this about 2 months back: cakephp/cakephp#2596
@bcrowe We can always change the CS to match 😉
There was a problem hiding this comment.
I'm fine with not having the semicolon. There is a technical reason for keeping them though, at it is that php will trim new lines (or was it preserve?) after the closing ?> if a semicolon is used.
There was a problem hiding this comment.
@lorenzo Must be UNIX thing then, as I made a quick test for Windows8 (PHP5.4) and it did not change behavior at all. One newline is always swallowed (after the closing tag).
There was a problem hiding this comment.
I don't remember the details of how it used to work :S
There was a problem hiding this comment.
I am fine with no semicolon either. We should however, be showing h() being used on any values coming from user input. That can be a separate pull request though.
There was a problem hiding this comment.
@markstory Or more appropriately: All user input and all stringish non-HTML (db) values outputted (to avoid some characters breaking the HTML).
|
I think this looks fine to merge as it stands. |
There was a problem hiding this comment.
"tmp" is now moved out of "App", so this should be just tmp.
Merge pull request #1137 into 3.0
|
Rebased and merged in 2f1e247 |
7 participants
I will comment on some lines. Looking for feedback on this.