Skip to content

provide possible solution feedback for discouraged functions #101

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

Closed
wants to merge 2 commits into from

Conversation

philippsander
Copy link
Member

No description provided.

@magento-cicd2
Copy link

magento-cicd2 commented May 22, 2019

CLA assistant check
All committers have signed the CLA.

@lenaorobei lenaorobei requested a review from paliarush May 22, 2019 13:50
@philippsander
Copy link
Member Author

I'll fix the CI fails soon

@fooman
Copy link

fooman commented May 29, 2019

I think dirname and basename are not a good case for making people use \Magento\Framework\Filesystem\Io\File::getPathInfo. The given better uses rely on an implementation detail (the returned array happens to conform to the pathinfo output) with the array having dirname and basename as keys. But this is not really part of any @api guarantees.

I believe it would be better to fix the framework first, either by adding extra methods ie getDirname and getBasename or by adding an optional input to pathinfo($filePath, $args = null) and then advise people to use those stable implementations.

@lenaorobei
Copy link
Contributor

I agree with @fooman here. Neither \Magento\Framework\Filesystem\Io\File nor \Magento\Framework\HTTP\Adapter\Curl are marked with @api, so I'd avoid such recommendation for now.

@lenaorobei
Copy link
Contributor

Have to close this PR since we cannot recommend using non-api classes.

@lenaorobei lenaorobei closed this Jun 24, 2019
magento-devops-reposync-svc pushed a commit that referenced this pull request Oct 19, 2021
AC-1314: Fix copyright sniff to check also less files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants