search

oroinc / orocommerce

Main OroCommerce package with core functionality.
http:///www.orocommerce.com/
Other
193 stars 84 forks source link

Simple native raw string replacement with PHP best practices through … #129

Closed DocFX closed 3 years ago

DocFX commented 3 years ago

…a constant

This version requires Symfony 4.4.*, which requires PHP 7.1.3+, which is over 7.1.0 for class constants visibility.

Could be the same for container parameters one day, but they're keys, not simple strings.

orocla commented 3 years ago

Thank you for your pull request.

It looks like this may be your first contribution to an Oro, Inc. open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://oroinc.com/b2b-ecommerce/contributor-license-agreement/

If you've already signed the CLA, it's possible we don't have your GitHub username or you're using a different email address. GitHub uses the email address you set in your local Git configuration to associate commits with your GitHub account. Please sign the CLA again using the correct GitHub username and email address or see this help article on setting the email on your git commits.

Once you've signed the CLA, please allow for some time for the submission to be processed.

mbessolov commented 3 years ago

@DocFX why introduce a constant that is used only once?

DocFX commented 3 years ago

@DocFX why introduce a constant that is used only once?

Some constants are never used there are actually some even in Symfony or ORO that might never be (like file modes in file components, or some old constants, even in PHP itself). :)

The principle is not about the use (though you're right about the number of uses - see https://rules.sonarsource.com/php/RSPEC-1192), it's about string literals being replaced (unless they're internal strings, like service names, which are OK to let as is). Which is a commonly accepted good practice.

DocFX commented 3 years ago

Also, no link, but... Why is the CI failing cause of another PR?

anyt commented 3 years ago

Why is the CI failing cause of another PR?

There was an issue in a master branch. It's already fixed. Back-merge will make the build green.

anyt commented 3 years ago

(unless they're internal strings, like service names, which are OK to let as is)

This was exactly the case, as the getName method was used internally by twig. But now it's a dead code. As of Twig 1.26, the \Twig\Extension\ExtensionInterface::getName() method is deprecated, and it is not used internally anymore.

So I don't see any value of this change. We should review extensions instead and drop unused getName methods. Internal ticket id for removing getName is BAP-20405.

I'm closing the PR.

anyt commented 3 years ago

Thank you for your pull request, but I'm closing this PR as we will not merge it for the reasons described in a previous comment.