Remove Deprecated Global Translate Function "__()"

[Sdfendor]

Description (*)

Removes the global translation function being deprecated since 1.3 (March 2009). found one remnant usage in lib/Varien which has no corresponding translation string thus in my understanding doesn't work anyways. This ancient artifact shouldn't have a place in 20.x.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[fballiano]

agreed

[fballiano]

I won't be easy tho, since I'm checkin a real life project and some external modules (from famous vendors) use the "__" global function in many places so I'll probably end up recreating this function anyway...

rethink

[fballiano]

Thinking about it... do we really want to break a lot of 3rd party modules for this cleanup?

[Sdfendor]

I'm really suprised about that because there are two other methods to do it already (Mage::app() and helper abstract thus any helper). I thought after all this time this wouldn't be the case 😕. I thought deprecations are there to be cleaned up eventually.

[justinbeaty]

Maybe I can add a detection and/or fix mode to #2332 to find places where the global function is called. It would be a similar manual upgrade path to #1619 #1470.

[justinbeaty]

I did push a php shell/translations.php deprecated --fix command in #2332. It uses regex so it's possible it breaks somewhere, but it correctly fixes the one case in lib/Varien. Maybe @fballiano if you can test that it fixes 3rd party modules in your case.

Either way I'm indifferent about removing the deprecated function, but figured it was a decent feature to add to the translations shell script.

[Sdfendor]

Thinking about it... do we really want to break a lot of 3rd party modules for this cleanup?

@fballiano That is one reason I target the 20.x branch. As I understand it the main purpose of 19.x is BC and the main purpose of 20.x is progress and the future . On one hand you are an advocat of progress which clearly shows in projects like #2270, on the other hand we can't remove code that is deprecated since 1.3 because of external modules?
Don't get me wrong. The main reason for my drive to remove this code isn't so much because of the deprecation or because it is a function outside of a class per se (the opposite is true, I see value in not having everything OO by default), it is because of the arbitrariness of it. What is part of functions.php and what is better placed in core helpers/the Mage class/Varien etc. follows no clear structure in OpenMage.
That's why I for instance deprecated and replaced global now() with Varien_Date::now() (#1991, #2250) (and I intend to remove the function in the future as well). Or take other functions like uc_words or is_empty_date. Why are those global functions while there are core helpers or even Varien components for strings and dates?
If the only argument for being part of functions.php is legacy then I think it's a weak argument at best. Do you get where I'm coming from?

@justinbeaty Thanks for your effort. It is a nice addition. I really appreciate it.

[fballiano]

I am, but, if we remove 3 lines of code we break every commercial code... then it could be not worth it. If it was a big performance improvements or dropping a thousand files from the repo, then let's go.

Don't get me wrong, I like clean code and cleanups but, in this specific case, it seems we could get an extremely small benefit but causing a big trouble to everybody.

I'm also thinking about this, nowadays a few commercial vendors still sell M1 modules, but technically a new OM project could still buy modules and use them without zero problems (until php8 I never had to patch anything for any of my customers) but if we introduce users will buy modules and they'll not work (at least from 3 very big vendors). This could have two outcome:

• people will think they can't use any M1 module anymore and we could lose that part of the ecosystem
• vendors will update those modules and better support OM
  risky move, it could pay off or not

Just saying it's better to think about this and weight pros and cons accordingly, I'm not ready to approve it right away let's say.

Hope I could better explain my concerns.

[elidrissidev]

I'm only worried about obfuscated extensions, the ones you have access to their code can always be updated in a few minutes.

[tobihille]

I joined the Magento eco system when 1.6 was the current version and I'm since then part of it and I have never seen any module that uses __() so I really don't get your point. I have no exact number but I guess that I have seen roughly 200 extensions since then.

Could you please point to some examples (that are not really outdated (speaking of outdated in the Magento 1 space is a stretch, I know 😅))?

Did I get it right that the 20.0-Branch is there to not hassle with backwards compatibility and make the code as clean as possible? If yes: would @Sdfendor's change not perfectly fit into this branch?
I perfectly understand, that backwards compatibilty is a big concern and I appreciate that you're maintaining it in the 19.0 branch.

[Sdfendor]

I admit that I don't know the full range of modules out there. I know some, even commercial ones and not only small ones. I took the experience from them and found IIRC one usage of the global translate function that needed to be changed.
The commercial angle especially with obsfucated modules is a valid one, I totally understand. Are we talking about still maintained commercial modules or those that are sold but no longer get updates? Or is it a mix of both? With the no longer updated ones their lifetime seems limited to me anyways in light of e.g. something like your mentioned vision for v21 that includes more significant changes.

In light of the commerical modules factor and new customers that not necessarily are able to fix incompatibilities between v20 and a module that support the latest Magento 1 version officially, the impact of this change (php footprint with one less defined function & cleaner codebase) seems indeed marginal.

Would the situation be improved if this PR targets an even later version of OpenMage (I'm fully aware that the plan regarding v21 isn't complete yet)?

[elidrissidev]

Some usages I found:

• Nwdthemes_Revslider
• Amasty_Helpdesk
• Webkul_Marketplace
• Magestore_Webpos

[fballiano]

Could you please point to some examples

Sphinxsearch by Mirasvit, Enhanced Commerce by Anowave, CustomerAttributes by Amasty

Did I get it right that the 20.0-Branch is there to not hassle with backwards compatibility and make the code as clean as possible? If yes: would @Sdfendor's change not perfectly fit into this branch? I perfectly understand, that backwards compatibilty is a big concern and I appreciate that you're maintaining it in the 19.0 branch.

I explained in detail it in my last comment. I'm not opposing the change but I think it's something to think about and weight pros and cons accordingly.

[justinbeaty]

If/when #1470 is merged, then obfuscated modules will likely be broken anyway with no way to fix. Unobfuscated modules will be broken until luigifab's script is run. I would say it's okay to merge this into the same release, perhaps the first release of a new major version (21.0 or 22.0) and have release notes mentioning how to migrate.

[tmotyl]

A possibility would also be to pack stuff like that into a separate module providing deprecated stuff back ? Not sure if its worth the effort

The base branch was changed.

[elidrissidev]

I approve this change if it targets next branch instead.

[addison74]

Maybe in the README we should have a section dedicated to the BC that we can cause through the adopted changes. Not everyone reads what a new version contains, not even the README, but if problems of this kind may occurred, they should be directed to a specific section.

Regarding this PR. the fact that you have identified around 8 extensions that still use that deprecated function makes me afirm to keep it there. If removal is mandatory, a solution must also be provided, first of all a section in the README file of the OM project and the solution for changing those extensions. I can't help but notice that some developers didn't make minimal efforts to keep their extensions updated.

[fballiano]

rebased on next, maybe we can evaluate if merging now?

[elidrissidev]

Agree from my side

[Sdfendor]

In actuality closing this PR has been collataral damage. I had to delete my fork of OM and recreate it (the exact reason escaps me right now) and this deleted in turn my open PRs. I totally overlooked this.
@fballiano By now we have the beta for 21. Is it worth revisiting the changes in this PR? There was an approve before the accidental deletion after all and you mentioned the next branch in a comment yourself.

[fballiano]

@Sdfendor you could download the patch for this PR here https://github.com/OpenMage/magento-lts/pull/2333.patch and open a new PR (when you delete the repo the original PR can't be reopened) using that patch for easiness, although the change to lib/Varien/Io/Sftp.php should be removed I think ;-)

[Sdfendor]

@fballiano Ok thanks for the patch file. About the change in Sftp.php: It uses the global function we are removing though. Wouldn't not removing this call break the error handling here (when the exception is thrown)? Varien_Io_Sftp is in no namespace so there is no chance another __() function could be called but the global one we would remove with the PR. Do I overlook something?

[fballiano]

ah sorry my mistake about the Varien_Io_Sftp!

[Sdfendor]

Patch applied and PR "reopened" with #3953.