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 visibility to __construct method in example #138

Merged
merged 2 commits into from
Apr 18, 2023

Conversation

jlnarvaez
Copy link
Contributor

Purpose of this pull request

This pull request (PR) add the correct visibility for the example adding a factory in __construct method.

Affected pages

@jeff-matthews jeff-matthews added the technical Updates to the code or processes that alter the technical content of the doc label Apr 12, 2023
@jeff-matthews jeff-matthews self-assigned this Apr 12, 2023
@jeff-matthews jeff-matthews self-requested a review April 12, 2023 15:23
Copy link
Contributor

@jeff-matthews jeff-matthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @jlnarvaez!

When the scope of a function is not defined, it defaults to public, right? Is there a reason to set the scope to public?

https://www.php.net/manual/en/language.oop5.visibility.php#language.oop5.visiblity-methods

@jlnarvaez
Copy link
Contributor Author

Yes, the default is public, this is true, but the recommendetion by PSR-12 is:

Visibility MUST be declared on all methods.

(https://www.php-fig.org/psr/psr-12/#44-methods-and-functions)

@jeff-matthews
Copy link
Contributor

Thanks for clarifying @jlnarvaez.

This will need to wait until we update the coding standard in #141.

@jlnarvaez
Copy link
Contributor Author

@jeff-matthews if not merged yet, this is a inherited standard from the current (and deprecated) standard PSR-2: https://www.php-fig.org/psr/psr-2/#43-methods

@jeff-matthews
Copy link
Contributor

@jlnarvaez, aha! I see. Thank you for clarifying. I'll go ahead and merge this then.

@jeff-matthews jeff-matthews merged commit fc29a8b into AdobeDocs:main Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical Updates to the code or processes that alter the technical content of the doc
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants