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

Add description why php functions marked as discouraged and give alternatives #102

Open
ihor-sviziev opened this issue May 24, 2019 · 6 comments
Labels
accepted New rule is accepted enhancement Improvements to existing rules Progress: ready for dev

Comments

@ihor-sviziev
Copy link
Collaborator

ihor-sviziev commented May 24, 2019

Preconditions

N/A

Steps to reproduce

  1. Make PR to Magento with changed file, that don't have like about ignoring discouraged pathinfo function or run magneto coding standard against some file.

Expected result

  1. You should get understandable message what's wrong with your code, message The use of function pathinfo() is discouraged isn't clean enough
  2. For case when some function marked as discouraged - there should be some information that shows reason of it, what is best practice
  3. That's actually relevant for all rules

Good example: https://github.com/extdn/extdn-phpcs/blob/master/Extdn/Sniffs/Classes/StrictTypesSniff.md

Actual result

  1. I'm getting result like this:
    Errors is quite clear, but not warnings
PHP Code Sniffer detected 2 violation(s): 

FILE: /var/www/html/app/code/Magento/Deploy/Service/DeployPackage.php
----------------------------------------------------------------------
FOUND 2 ERRORS AND 2 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------
 109 | ERROR   | [x] Missing short description
 110 | ERROR   | [x] There must be exactly one blank line before tags
 224 | WARNING | [ ] The use of function pathinfo() is discouraged
 225 | WARNING | [ ] The use of function pathinfo() is discouraged
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------



Failed asserting that 2 matches expected 0.

Related question on stack overflow, answer not seems to me correct.
https://magento.stackexchange.com/questions/192978/the-use-of-function-pathinfo-is-discouraged-magento-2

@ihor-sviziev ihor-sviziev added the bug Something isn't working label May 24, 2019
@gixid192
Copy link

Totally agree. I wanted to follow the rules but sometimes it's hard to find an alternative function.

Usually, my approach is searching for that function in the whole vendor/magento folder, see if the Core team use it else where in the code. If I can't find it, I got stuck.

I also know that some are just warning, it is there to help developers mind the usage of the function, for example base64_decode, there is no wrapper, but it is safe to use with trusted input.
But the help with a suggestion will save developers time finding, say, in this case, the pathinfo can be found here https://github.com/magento/magento2/blob/2.3-develop/lib/internal/Magento/Framework/Filesystem/Io/File.php#L890

@lenaorobei lenaorobei added accepted New rule is accepted and removed accepted New rule is accepted labels May 31, 2019
@lenaorobei lenaorobei added accepted New rule is accepted enhancement Improvements to existing rules and removed bug Something isn't working labels May 31, 2019
@diazwatson diazwatson self-assigned this Aug 3, 2019
@diazwatson
Copy link
Contributor

You can use Magento\Framework\Filesystem\Io\File which has a wrapper for that function.

Example:
https://magento.stackexchange.com/questions/192978/the-use-of-function-pathinfo-is-discouraged-magento-2/284317#284317

@ihor-sviziev
Copy link
Collaborator Author

ihor-sviziev commented Aug 3, 2019

Hi @diazwatson,

You can use Magento\Framework\Filesystem\Io\File which has a wrapper for that function.

Example:
https://magento.stackexchange.com/questions/192978/the-use-of-function-pathinfo-is-discouraged-magento-2/284317#284317

Looks really good! Thank you!

However it adds alternative only for one function.
This issue was created in order to provide some alternative for all discouraged functions, it could be done even with documentation that describes motivation why each function was marked as discouraged and what should be used instead.

Are you interested in adding such documentation?

@diazwatson
Copy link
Contributor

Sure, np I will add those

diazwatson added a commit to diazwatson/magento-coding-standard that referenced this issue Aug 3, 2019
@diazwatson
Copy link
Contributor

@ihor-sviziev I just reviewed the list of $forbiddenFunctions and is huge (187 functions are forbidden).

Also not all of them have an equivalent or wrapper in Magento.
Should we add them to Magento2 repo?

Anyway I have added some more alternatives for discouraged functions

diazwatson added a commit to diazwatson/magento-coding-standard that referenced this issue Aug 6, 2019
diazwatson added a commit to diazwatson/magento-coding-standard that referenced this issue Aug 9, 2019
diazwatson added a commit to diazwatson/magento-coding-standard that referenced this issue Aug 9, 2019
diazwatson added a commit to diazwatson/magento-coding-standard that referenced this issue Aug 10, 2019
lenaorobei added a commit that referenced this issue Aug 12, 2019
#102 Add alternative for php pathinfo function is discouraged
@lenaorobei
Copy link
Contributor

Keep this for future recommendations enhancement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted New rule is accepted enhancement Improvements to existing rules Progress: ready for dev
Projects
None yet
Development

No branches or pull requests

4 participants