Convert long form tags with echo to use short-echo tags

[davidalger]

There has long been a community request (see #107) to see Magento use short-echo tags across the board. Many prominent members of the community are in support of this change, particularly since system requirements were updated to demand PHP 5.4 or newer. Do note that this is not to be confused with short tags in general, and nobody is suggesting Magento use short tags, only short-echo tags. So the only thing being done here is replacing instances of <?php echo $foo ?> with the shorter <?= $foo ?> form which as of PHP 5.4 is fully supported.

The largest benefit of this is code readability. There is also a benefit of reduced line lengths. Templates are notorious for cumbersome line-lengths and this will help alleviate that to a degree.

The conversion seen here was done using the script found in this gist: convert-tags.sh. Very simple to do actually.

Two things to note about this PR since it has a pretty wide scope:

a. Somebody would need to run the script agains the enterprise code to effect the change there as well.

b. Since it affects all templates, I am aware there is a risk of conflicts between this and other active branches internally. If conflicts arise, they are easily resolved (the history on the branch of this PR will show this as having been done once):

git checkout feature/short-echo-tags
git fetch --all
git merge upstream/develop -X theirs
bash -c "$(curl -sSf https://gist.githubusercontent.com/davidalger/fc88849cb1451589856a/raw/c848ee5922736c0f7dc4e930be4053572c913721/convert-tags.sh)"
git push

One question which I think this begs is this: Should I write a static test to enforce the use of short-echo tags in the core. Yes, PSR-1 allows them both. But it would be great to see Magento not only using them, but have something in place to enforce consistent use of them throughout the core and across the board. If 3rd parties don't want to use them, fine…their choice…but at least core has one unified format. I'd be willing to add a static tests for this if desired.

Please consider accepting these changes. The community will thank you for it! ;)

[steverobbins]

Can we write a code sniff for this? There is already one to disallow <?= but not to enforce it.

[vancoz]

Hello @davidalger, according to latest @maksek response here #107 we are going to keep full tag as main one.

[Vinai]

What a pity. The reply of @maksek only mentioned restrictions on the short tag, not the short-echo tag, on which he even mentioned that it is always available since PHP 5.4 and can't be turned off. Plus, @davidalger did great work. If there is additional testing/checking to be done in order to accept this PR, maybe it can be passed on to the community. There seems to be a lot of support for this in the community.

[erikhansen]

@maksek @vancoz If you look at the PSR-1 section regarding tags, you will see that it says:

PHP code MUST use the long <?php ?> tags or the short-echo <?= ?> tags; it MUST NOT use the other tag variations.

Since short echo tags are very clearly acceptable as a part of PSR-1, I would ask you to reconsider your stance on this PR, as I think the community (and programmer's fingers across the world) would thank you for it.

[vancoz]

@erikhansen, davidalger we are really appreciate contribution from our community so let us discuss this question again.

Regards

[orlangur]

@davidalger are you going to enforce it with sniff within this PR?

[davidalger]

@orlangur I've made one pass at adding a static test. I'm waiting on builds to complete, then going to push some code which should intentionally break the test to verify it fails a changed file. Once I do that, I plan on reverting the failed change and then hope to see this accepted!

[davidalger]

Looks like #9872 just got merged into the mainline, so I just rebased on develop and pushed. Let's see if my tests still pass! :)

[orlangur]

Changes seem fine to me but why need them in scope of this PR?

[davidalger]

@orlangur When you ask "why need them in scope of this PR" — what changes are you referring to?

[orlangur]

@davidalger it was a626c36, I expected to have this comment attached to particular changes..

[davidalger]

@orlangur Ah, ok. I think I understand now :) The changes to develop caused conflicts in a few files, resulting in GitHub showing the branch as not-mergeable. So I merged upstream/develop using -X theirs and ran the script from the original task description to update those templates which, as a result of the merge from develop, now had usages of <?php echo on the lines changed. These lines would have failed the static tests after I merged if I didn't update them. Does that make sense? Basically, I was updating the branch so it would be mergeable. Maybe there is a better way to handle that?

[orlangur]

@davidalger thanks, got it, so, basically those semantical changes came from mainline, not from your PR.

Sorry for confusion, I saw a3ee58a + a626c36 in one screen just because so GitHub notifications work. This mistake should not occur again)

[davidalger]

@orlangur No problem. Thanks for taking this!

[okorshenko]

Hi @davidalger we are working on stabilization of this PR.