Skip to content
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

Use short echo tags in phtml templates #107

Closed
colinmollenhour opened this issue Oct 25, 2012 · 65 comments
Closed

Use short echo tags in phtml templates #107

colinmollenhour opened this issue Oct 25, 2012 · 65 comments
Assignees

Comments

@colinmollenhour
Copy link

PHP 5.4 has made the short echo tag <?= always available regardless of the short_open_tag setting so this feature is now 100% future proof and using it in templates reduces quite a bit of unnecessary typing/characters. The feature has been in PHP forever, only before 5.4 it could be disabled in php.ini.

Proposal example before:

<dt><?php echo $this->getFoo() ?></dt><dd><?php echo $this->getBar() ?></dd>

Proposed change to:

<dt><?= $this->getFoo() ?></dt><dd><?= $this->getBar() ?></dd>

I'd be happy to tackle this by writing a script which can do the replacement with a regex. I think it won't be hard using a lookahead to make sure the replacement is only done when there is one statement between the open and close tags (detected by semicolons).

Note, I am not proposing changing all tags to short tags, just the ones that contain a single echo statement on one line.

If you approve I will start on the script. Actually I would probably use DAMit which would make it easy for everyone to convert their code if they so desired:
http://code.google.com/p/dam-tools/source/browse/trunk/DAMit.pl

EDIT:
For users of PHP versions prior to 5.4 it should be possible to force short_open_tag to "on" via .htaccess.

php_value short_open_tag 1
@IvanChepurnyi
Copy link
Contributor

I would love to... But how about minimal system requirement of PHP5.3? PHP5.4 is not yet so popular...

Also initial public version of Magento was with short_tags, but because of issues with on most of installments with turned off directives it was changed to <?php echo ...

@daim2k5
Copy link
Contributor

daim2k5 commented Oct 25, 2012

-1 It think the default value is off for shortcuts

@Schrank
Copy link

Schrank commented Oct 25, 2012

I prefer short tags too, but Ivan is right, many Magento versions are still running on PHP 5.2

But the comments after me are right.

If all the things are updated, it is a good idea to look forward. Using all the cool new features in PHP 5.3 and 5.4 would be great!

Let's change it!
+1

@Vinai
Copy link
Contributor

Vinai commented Oct 25, 2012

Magento 2 is supposed to be open to embrace change and the future. It's better to change the minimal requirements to 5.4 in my opinion. Someone running Magento2 on 5.3 should have no problem enabling short open tags.
The biggest benefit in my eyes is improved readability.

👍 from me!

EDIT: In case you missed it, in PHP 5.4 the "<?= is now always available regardless of the short_open_tag setting." (taken from http://www.php.net/ChangeLog-5.php#5.4.0 in the General Improvements)
We probably all agree that Magento should not be made with unspecialized shared hosts in mind.
I don't see a problem with 5.4 as a requirement if the host is setting up a tuned Magento environment anyway.

@centerax
Copy link
Contributor

I totally agree with Vinai. +1

@Flyingmana
Copy link
Member

+1 because a lot of templates I saw were not readable because of the overload of php code

@johnholden
Copy link

To play the devil's advocate... Zend forbids them in their coding standard: http://framework.zend.com/manual/1.12/en/coding-standard.coding-style.html

@pavelnovitsky
Copy link

  • it won't make templates more readable
  • until 5.4 is not the minimal requirement this change will cause a lot of questions and broken keyboards. If you think that there is no any problem in enabling short syntax — remember flood of repetitive questions like "switched from php 4 to php 5, site does not work"
  • enabling short syntax in php does not make it mandatory
  • what about "Short tags were, for a time, the standard in the PHP world; however, they do have
    the major drawback of conflicting with XML headers and, therefore, have somewhat
    fallen by the wayside"? Or I do not know and this issue was solved?

On the other hand almost the same can be told about GD2, PDO, etc.

But any way -1

@mzeis
Copy link
Contributor

mzeis commented Oct 25, 2012

I'd like the short echo tags for better readability but this shouldn't be the only reason to bump the requirements up to PHP 5.4. Make use of new features like traits and I'm in for 5.4.
Another reason for PHP 5.4 would be to finally forget about register globals, magic quotes and safe mode.

As for support: Debian Wheezy e.g. will use PHP 5.4.4. That being said it will be a while before PHP 5.4 is widely used by providers. If you plan to release Magento within the next year, there may be problems regarding the PHP version.

@Vinai
Copy link
Contributor

Vinai commented Oct 25, 2012

@Vinai
Copy link
Contributor

Vinai commented Oct 25, 2012

@luge The XML parser compatibility problems where caused by the <? opening tag. <?= wasn't the problem.

@benmarks
Copy link
Contributor

To johnholden's point regarding the proscription of short tags in ZF, that is for ZF1 which evolved when short tags might not be available. However, it seems that ZF2 will allow the use of short tags in view scripts.

@LeeSaferite
Copy link

+1 on short tags and +1 on 5.4 min version requirement.

As was already mentioned, a version bump to 5.4 would be great as you get things like traits which would be highly useful in Magento.

@johnholden
Copy link

@benmarks This is true, but it appears that Magento 2 with be using ZF 1.11, from what I see here on Github.

That said, I'm not opposed to short tags--I just like to keep things consistent. I've spent the last 10 years removing/rejecting short tags because that's what most coding standards have been calling for.

Also there's nothing stopping any individual or agency from using short tags in templates if they prefer.

@SchumacherFM
Copy link
Member

+1 for short tags and also for the 5.4!
and +1Mio for ZF2 ;-)

@barbazul
Copy link
Contributor

👎 For short tags
👎 For 5.4 requirement
👍 For ZF2

Although I am afraid switching to ZF2 at this point might involve pushing the final release date even further. I'd rather have Magento2 running on ZF1 rather than not having it at all so..... 👎 for ZF2

@nhp
Copy link

nhp commented Oct 26, 2012

+1 for short echo tag (no general short tags)
Moving to 5.4 could be a bit harsh because of many providers will take their time to make the switch to 5.4 and that could become a show stopper for smaller shops that try to upgrade/install Magento2. In General it would be a good choice but as mzeis said not only for usage of short echo tags but maybe for general features. ZF2 should be the way to go even if it only is because of maintainability.

@leesbian
Copy link

👎

This shouldn't be a matter of opinion, it should be a matter of pragmatism (as should pretty much all decisions of this type)

Unless PHP 5.4 is a requirement for Magento2, it's a bad idea - it's just one more thing to trip people up when setting up servers or local development environments.

Don't forget, not everybody will be able to enable short tags if they're not using PHP 5.4 - perhaps they don't admin their servers, or it's a shared server (either internally, or via a hosting provider), or it could be a cloud image that is non-editable by the customer etc.

I've spent a lot of time berating developers who use short tags, and removing them from their code - it's bad practice - it doesn't conform to Zend coding standards (so will fail any CI coding standards checks) and also leads to the inevitable "but it works on MY computer syndrome" - all Bad Things.

Besides which, if you allow short tags in templates, you have to allow short tags everywhere for the sake of consistency - anything else would be an invented convention for the sake of it - another Bad Thing.

Anybody wishing to continue using short tags can always configure a text expansion macro in their IDE, the only remaining issue is readability of templates - I personally find it MUCH easier to read the long form

As for ZF1 vs ZF2 - the choice is a simple one - if ZF1 will become EOL within the expected lifetime of Magento2, then ZF1 should be dropped in favour of ZF2 now. Same argument for PHP 5.3.

@Vinai
Copy link
Contributor

Vinai commented Oct 26, 2012

@nhp Probably the bulk of migrations will happen 2014 or later. Until then 5.4 will be much more common place then today.
As said before, Magento shouldn't be hosted in general purpose environments anyway. There are plenty of cheap shared hosts that have Magento specific offerings. For those 5.4 won't be an issue, since they are maintaining a special stack for Magento anyway.

Using a merchant who wants to install in the cheapest shared host available isn't a good smallest common denominator.

This really is a chance to open the doors for new possibilities and break with backward compatibility.
If 5.4 is chosen as a minimum requirement all the other PHP features it brings with it can (and will) be used by module developers, and who knows - maybe even the core team :)

Sooner (5.3) or later (5.4) the time will come when we will feel weighted down by supporting an ancient PHP version with Magento 2 :) Lets make that later.

@pavelnovitsky
Copy link

So discussion changed from "short echo tag" to "Make 5.4 as a minimum requirement"

Again 👍 for 5.4 and traits, 👎 for short echo tag

@Vinai
Copy link
Contributor

Vinai commented Oct 26, 2012

@leesbian I think a few of your assumptions are a little flawed.

I'm with you regarding the point on pragmatism though, even if it brings me to a different conclusion :)

The argument regarding the setup of a Magneto dev host with 5.4 tripping up folks is unlikely. C'mon, we aren't talking about the "generic PHP scripter", but rather Magento Developers, who should be able to figure out a PHP version setup. "Scripters" messing around in a complex system shouldn't be of concern.

Not everybody being able to enable short open tags on 5.3 hosts is just as saying Magento depending on mcrypt is a problem. It's just a system requirement, nothing more. repeat-my-argument-against-inflexible, non-magento-specific-hosting-solutions

If (big IF here) 5.4 is the minimum requirement for Magento 2, then the "it-works-on-my-computer" argument won't happen, since short echo tags will always work. They can't be disabled.

The argument of having to allow short open tags everywhere if they are allowed in templates, well, we aren't talking about short open tags <? (which can be disabled), but short echo tags <?= (which are always on).

@luge I agree that I'm (miss?)using this thread to push for 5.4. Still personally I also think that short echo tags are cool :)

@leesbian
Copy link

@Vinai your point about "it works on my computer" is exactly the same point I made, which is why I was supporting the move to a PHP 5.4 requirement.

However, I think the deal with people not being able to enable short tags on PHP 5.3 is massive - especially as we move further towards cloud environments such as ElasticBeanstalk where the customer has no control over the PHP runtime.

Magento users shouldn't NEED a "Magento specific" environment. There shouldn't be a NEED for "Magento specific hosting solutions". Those only exist at the moment because Magento is a bloaty resource hog - I would hope that Magento2 is addressing those issues - cure the disease, not the symptoms :)

Anyway - make PHP 5.4 a requirement, then it's up to the individual what they want to do - either method is valid, nothing breaks, no special setup required.

@benmarks
Copy link
Contributor

I had not intended to inject the topic of ZF2 as a Magento 2 library in my earlier comment. I was merely using it as an example of an ecosystem-neighbor framework allowing short echo tags (ty Vinai for reminding us of the distinction). That discussion does not belong here. In general, I trust that the core team will balance their work and timelines against the expected lifetime of Magento2.

I read @leesbian 's comment and came to say much of what @Vinai just said. I'll add that, in response to Lee's point that "if you allow short tags in templates, you have to allow short tags everywhere for the sake of consistency", well - ZF2 is doing just this, and the reasoning is readability. Certainly this is as subjective a topic as "which brace style to use" and should receive input from us all.

Regarding PHP 5.4 and Magento 2: this is germaine (where ZF2 isn't necessarily) because it makes the use of short open tags in templates tenable. My $0.02 is that whereas PHP 5.4 was released March 1, 2012, and whereas Magento 2 will not be officially released for some time now, making 5.4 a minimum requirement is not only possible, it will prove to be timely and advisable on its own merit, regardless of the short echo tags issue.

@colinmollenhour
Copy link
Author

Thanks for the comments, all.

I haven't tested but it should be possible to switch on short tags setting in the .htaccess file to circumvent issues with users or hosts clinging to 5.3.

@barbazul
Copy link
Contributor

This dicussion is starting to feel a little like the namespaces separators one.

I personally don't like short open tags and find them counterintuitive and more difficult to read and/or detect than <?php echo

But @Vinai and @benmarks make a good point regarding pushing forwards for new versions and features.

I got me thinking... Magento IS (or should be) the number one ecommerce platform so it should definitely be considered as one f the game changers platforms hosting companies should be looking at when deciding what services to provide. Other software that falls into this category: Drupal, Wordpress, etc. The PHP community has always suffered from this "hold back" syndrome and software platforms of this should take the blame, not hosting companies. Nevertheless the release schedule for magento2 is still blurry and as far as we know they could release tomorrow (not likely) or in 3 years from now, so although I am changing my vote to 👍 PHP 5.4 compatibility (in case somebody is counting) i would not race to implement traits and short echo tags yet to keep the possibility open to change back to 5.3 easily in case of an earlier than expected release.

ZF2 Is a different story. @leesbian makes an excelent point. There simply is no other option than making the switch now. It's not a matter of opinion. It's a little extra work now to make magento2 live longer

👎 For short tags
👍 For 5.4 compatibility (and future requirement)
👍 For ZF2

@gfxguru
Copy link

gfxguru commented Oct 26, 2012

+1 on php 5.4 ... and I would like to know how to circumvent now. We've updated to 5.4 because of security reasons, bug fixes, etc. I believe an incompatibility with 5.4 is unacceptable.
-1 short tags
+1 ZF2

@colinmollenhour
Copy link
Author

Does anyone have any real examples of respectable hosts that otherwise support Magento that don't allow short_open_tag? I find it hard to believe there are any or at least enough that it would warrant holding back on this change. I've seen several community modules that use short echo tags for Magento 1.x and I haven't ever heard a complaint about it. If there truly was such a situation that one's host refused to allow short_open_tag (is this even possible?) then a simple command could change them all back (including any third-party modules):

$ find app -name \*.phtml -exec sed -i 's/<\?=/<?php echo/' {} \;

How to then copy all of these modified files back up to the said host using highly insecure FTP is an exercise left up to the fool who would use such a host in the first place. :-P

Some stats:

  • Key taps saved per occurrence: 7
  • Occurrences: roughly 15,000 (grep -roh '<?php echo' app | wc -l == 15192)
  • Total key taps saved: > 100,000

Holy arthritis, batman! That's a lot of highly verbose and redundant characters!

For the record this proposal:

  • Is not to use short open tags for non-echo statements. It is only for echo statements.
  • Does not break compatibility with any version of PHP. E.g. it does not require PHP 5.4. The feature has been present since god knows when.
  • Will conflict with the default settings for PHP 5.3 (short_open_tag should be changed to "on" in php.ini or .htaccess). In fact, short_open_tags is the default on many versions and I'm sure it will cause 0 problems to enable this.
  • Will never conflict with any settings in PHP 5.4 and up.
  • Doesn't have anything to do with ZF2 other than the fact that it is the same coding standard that has been adopted for ZF2, PSR-1 and other industry standards.
  • Is not a security risk in any way.

The biggest downside to this proposal is that with all of the time saved not typing "php echo" the Magento 2 team would have to fire one of their template designers. ;-)

@leesbian
Copy link

@colinmollenhour do you have button-phobia or something? ;)

Saving a few keystrokes isn't a particularly strong argument, especially when creating a text expansion macro in your IDE would have been less keystrokes than your last reply, wouldn't have required anybody to make changes to their server configuration, and achieves the same end result.

The PHP 5.4 issue has been raised because it plain DOESN'T MAKE SENSE to implement the short echo tags if it will cause more issues than it solves. Making PHP 5.4 mandatory for Magento2 ensures there is NO risk by implementing your suggestion, as it will ALWAYS work. After the text expansion solution, this is the simplest, most pragmatic solution. Just as @Vinai pointed out - PHP 5.4 then becomes a REQUIREMENT of Magento2.

The ZF2 issue has been raised because in an effort to justify short tags some people have mentioned future-proofing. We've already noted the ZF1 will become EOL well within the expected lifetime of Magento2, so for a consistent argument, we also need to accept that ZF2 is required in order to future-proof.

This then circles back to the PHP 5.3 v's PHP 5.4 debate - as it's highly likely that PHP 5.3 will be EOL well before the end of the expected lifetime of Magento2, and as PHP 5.4 is GA now - and offers the benefits of traits (that would HUGELY benefit Magento) - why would we not make the decision now to mandate PHP 5.4?

This then circles back to the whole LTS conversation - because at the moment, we can only guess at the intended lifetime of Magento2 (I'm assuming a minimum of 5 years, as it's common for implementation costs [for large projects] to be amortised over a 5 year period).

@colinmollenhour
Copy link
Author

What about maximum line length? The adopted rule is soft max of 80 and hard max of 120 although I very frequently see lines past 120 in Magento. Saving 7 characters per echo means fewer lines will exceed the maximum line length and need to be broken or the ones that still exceed will not be exceeding the maximum by as much. Look at any template file that has lines exceeding 80 characters and 95% of them will have at least one echo tag. Nothing hurts readability more than not being able to see the code on your screen!

As for requiring PHP 5.4 due to the impending EOL of PHP 5.3 this is a moot point since PHP 5.4 is still obviously supported. In the case of ZF2, as we have all seen with the xml security hole, if there is a security hole found Magento will provide the patch well before Zend does anyway so I really don't see a need for ZF2 as it will probably introduce more problems than it fixes.

@colinmollenhour
Copy link
Author

Visually compare php echo to short echo (catalog/product/price.phtml has 170 instances of '<?php echo'):
Long: http://screencast.com/t/VJHu92RIt6FG
Short: http://screencast.com/t/G0yhgk194I

@davidalger
Copy link
Member

@alankent if I take the time to update all the template files to use the short-echo tags and submit a PR, would it be something the team would consider accepting?

I would like to see them used across the board throughout the stock templates, but know it can take work to get it there. Currently they are used somewhat sporadically. They improve template readability IMO and thus I'm willing to take a shot at doing the heavy lifting on this if such a PR would be considered for acceptance.

@sshymko
Copy link

sshymko commented Jun 10, 2015

To whoever is going to review this request:

Keep in mind that this is a CE repository. A pull request would include changes to CE files only. Somebody from the Magento team will have to work on EE files in order to complete the short tags support.

@alankent
Copy link

I think more of a question for @maksek (the Engineering team). I worry more about big picture architecture.

Good question though. Is there a reason you prefer the short form? How many other people agree? If you get a bunch of people saying "its a good thing" with not many saying "its a bad thing" it makes decisions easier. Some changes are really "for the community". It does not affect us. If such a change makes the community happy, its good.

I will however also say that it may be easier to accept (due to merge conflicts) by providing a script to make the change, rather than a mega commit doing the change (and colliding with everyone else's work). But I will leave to Max.

I will also say as we get closer to GA the internal pressure will go up, like any project. The benefits of the work need to be clearer as every engineer looking at this sort of thing is not working on something else. We still need to code review any change request, which takes real time for a mega change like this. So personally I think it would need to be pretty compelling community vote that this is important to do.

@erikhansen
Copy link
Contributor

@alankent I would love to see all template code refactored to the short-echo tag syntax. In addition to saving keystrokes, it would also make it easier to scan a template and see which php tags output content vs everything else. In a quick search of the M2 codebase, I found ~21 usages of the <?= tag in non-vendor code. So it seems that some developers are taking advantage of the option to use the short-hand syntax, but I would love to see this documented as a standard, so that the codebase is consistent.

As an aside, it seems that the Magento 2 coding standards should be more opinionated about things like this in order to enforce consistency. Maybe Magento has more specific internal development guidelines that could be added to the public coding standards?

@davidalger
Copy link
Member

A quick review of the comment thread here reveals a dozen in support of short-echo tags and less than half that in opposition, at least a couple of the opposers being due to PHP 5.4 not being a requirement at the time (2-3 years ago).

@sshymko Good point. I could take care of that and make a PR on the EE repo too if that made a difference though. ;)

@alankent I'm curious to hear @maksek's thoughts on this. Personally I think it's a huge amount of benefit due to less syntax clutter in templates, leading to better readability and overall cleanliness.

I could provide a script to make the change. I used a few perl commands to replace everything, ran the lint checker on all the changed files to verify it didn't bust syntax and then pushed up e50d17d5b4 so I could run the test suite on travis and be sure it didn't totally screw them up. Now that it's run and I can see the result is the same as current results on the develop mainline, I feel comfortable posting the script.

Look forward to hearing others input here. I'd be happy to let your team use the script directly, happy to make the PR on the internal CE/EE repos so it'll get both without requiring effort to effect the change, or happy to just do it here and let you take the EE change internally via the script. Let's just say I'll be happy to see the change allowed/effected regardless of whether it's through a PR I submit or not. ;)

Here's what I ran to make the changes for my testing purposes…

#!/usr/bin/env bash

# using *html vs *.phtml because one .html file has a use of <?php echo in it
# the first expression converts all the single-line to a uniformly formatted
# short-echo tag format, the second updates the few existing short-echo tags
# to use the same padding applied in the first, and the third expression just
# replaces the opening tag on the few multi-line cases (where I did a visual
# check to ensure it wasn't touching things it shouldn't)

find . -type f -name '*html' -print0 | xargs -0 perl -pi -w -e 's#<\?php\s*echo\s*(.*?);?\s*\?>#<?= $1 ?>#g';
find . -type f -name '*html' -print0 | xargs -0 perl -pi -w -e 's#<\?=\s*([^\?]*?);?\s*\?>#<?= $1 ?>#g';
find . -type f -name '*html' -print0 | xargs -0 perl -pi -w -e 's#<\?php\s*echo\s*#<?= #g';

# there are actually a handful of cases in non-template files, get those too
find app/code/ -type f -name '*.php' -print0 | xargs -0 perl -pi -w -e 's#<\?php\s*echo\s*#<?= #g';

# verify we did not break syntax of phtml and php files
find . -name '*.phtml' -print0 | xargs -0 -n1 php -l | grep -v "No syntax errors";
find app/code/ -name '*.php' -print0 | xargs -0 -n1 php -l | grep -v "No syntax errors";

# revert changes made to 2 unit test files before comitting
git status -s | grep 'Test/Unit' | awk '{print $2}' | xargs git checkout
git commit -am 'Resolves #107: Replace all usages of <?php echo with <?= tags and unify padding on existing tags';

@jonathanhodges
Copy link

@alankent I also think moving to a consistent and shorter syntax, since PHP 5.4 is already a requirement, is a good idea. Also to the point @erikhansen was making, I believe that providing more coding standards (specific to the Magento architecture) could help reduce poor quality modules.

@alankent
Copy link

Understood. We are following PSR standards - there are occasional debates how much further we should go over PSR, where in this case PSR allows both forms. Max I think is travelling at present - he will probably jump on this thread soon.

@davidalger
Copy link
Member

@maksek Do you have any input on this?

@maksek maksek self-assigned this Jul 22, 2015
@maksek
Copy link
Contributor

maksek commented Jul 22, 2015

@davidalger Sorry for long time without feedback.

Lets see how it evolves.
PHP parses file with starting tag <?php, and it allows to use short tags <? as short to <?php and <?= as short to <?php echo.
Starting PHP 5.4 short tag <?= is always available regardless of the short_open_tag ini setting. At the same time, PHP discourage to use short tag <? because it is available only with short_open_tag enabled. Also PSR-1 does not allow to use short tag <?.
Analyzing short tag usage versus full tag usage we see there are some restrictions on using short tags, but no restriction to use full tag.
With this logic - I would keep full tag as main one, but do not restrict to use short one.

Magento 2 is OpenSource product, and we are value community opinion, so I am open for discussion :)

joanhe pushed a commit that referenced this issue Nov 12, 2015
…varnish-merchant-beta

[Extensibility] [merchant beta] MAGETWO-44438 clear varnish from CLI
davidalger added a commit to davidalger/magento2 that referenced this issue Jun 9, 2017
Also unify existing short-echo padding. Resolves magento#107. PR magento#1563.
magento-engcom-team pushed a commit that referenced this issue Apr 13, 2018
…underscore

Fix Call to undefined function Magento\Ui\Controller\Adminhtml\Index\_()
magento-engcom-team pushed a commit that referenced this issue Sep 14, 2018
magento-engcom-team added a commit that referenced this issue Oct 4, 2018
 - Merge Pull Request magento/graphql-ce#107 from magento/graphql-ce:product-url-rewrites
 - Merged commits:
   1. d6d0872
   2. b583ee7
   3. 57c2e4c
   4. 46a871e
   5. dfabfe5
   6. e0f4132
   7. e9a06a6
   8. b062bd9
   9. 1700740
   10. f141a0d
   11. e2cad89
   12. 16b72a8
   13. df3360b
   14. 987b138
   15. 0fe1ae5
   16. 7b8799d
   17. 6d8e43d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests