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

improve and make more strict autoload.php #28923

Merged
merged 1 commit into from
Aug 15, 2020
Merged

improve and make more strict autoload.php #28923

merged 1 commit into from
Aug 15, 2020

Conversation

rvitaliy
Copy link
Member

@rvitaliy rvitaliy commented Jun 29, 2020

Description (*)

two improvements:

  1. use is_readable instead of file_exists. if we use require or include on a existing file that is not readable php generate errors. So we need to chech if file_exists and is_readable (is_readable do both things)
  2. provide an anonymous function to determinate $vendorAutoload, it's allow us to have a more readable code with early return.

Related Pull Requests

none

Fixed Issues (if relevant)

none

Manual testing scenarios (*)

not necessary, if CI passes this code is good, it's only technical improvement.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] improve and make more strict autoload.php #29527: improve and make more strict autoload.php

@m2-assistant
Copy link

m2-assistant bot commented Jun 29, 2020

Hi @rvitaliy. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

@rvitaliy
Copy link
Member Author

@magento run all tests

@rogyar rogyar self-assigned this Jul 3, 2020
@rogyar
Copy link
Contributor

rogyar commented Jul 3, 2020

@magento run Functional Tests B2B

1 similar comment
@rogyar
Copy link
Contributor

rogyar commented Jul 7, 2020

@magento run Functional Tests B2B

$vendorAutoload = (
static function (): ?string {
$vendorDir = require VENDOR_PATH;

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @rvitaliy. Thank you for your collaboration. Could I kindly ask you to provide a comment for these two declarations of $vendorAutoload with highlighting what is a particular block for? At first glance, these blocks of code look very similar (almost the same) and it's hard to guess why we need such an approach. I see that in first case we have the trailing slash, but let's make developers life a little bit easier and highlight this case in the comments. Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

@rogyar
sorry, but i can't do it.
personally i don't undestand why we need the second check without BP prefix.
i'm refactor the old code without change to the logics, make it more strict and readable.
IMHO: we can remove the second block of check, app/{$vendor} is not a valid path for vendor.

Copy link
Contributor

@rogyar rogyar Jul 13, 2020

Choose a reason for hiding this comment

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

Good point. I see that this functionality has been recently added. So I assume it was done for a purpose.

2054fab

Unfortunately, there's no additional information about this change. Well then, let's leave it as it is. Maybe it's required for some particular circumstances (like CI system or so).

Copy link
Contributor

Choose a reason for hiding this comment

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

@rogyar, that commit is from #18849, so there is your missing information 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

so the second check is need to autoload vendors from the global path

@rogyar rogyar added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Jul 13, 2020
@ghost ghost moved this from Pending Review to Ready for Testing in Pull Requests Dashboard Jul 13, 2020
@magento-engcom-team
Copy link
Contributor

Hi @rogyar, thank you for the review.
ENGCOM-7830 has been created to process this Pull Request

@engcom-Charlie engcom-Charlie moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Aug 7, 2020
@engcom-Charlie
Copy link
Contributor

QA Not applicable

@engcom-Charlie engcom-Charlie moved this from Testing in Progress to Extended Testing (optional) in Pull Requests Dashboard Aug 7, 2020
@engcom-Charlie engcom-Charlie self-assigned this Aug 7, 2020
@engcom-Charlie engcom-Charlie added Component: Autoload QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels Aug 7, 2020
@sidolov
Copy link
Contributor

sidolov commented Aug 13, 2020

@magento create issue

@sidolov sidolov added the Priority: P3 May be fixed according to the position in the backlog. label Aug 13, 2020
@sidolov sidolov added Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it labels Aug 13, 2020
@engcom-Alfa engcom-Alfa added QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope and removed QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels Aug 14, 2020
@engcom-Charlie engcom-Charlie moved this from Extended Testing (optional) to Merge in Progress in Pull Requests Dashboard Aug 14, 2020
@magento-engcom-team magento-engcom-team merged commit ced4144 into magento:2.4-develop Aug 15, 2020
@m2-assistant
Copy link

m2-assistant bot commented Aug 15, 2020

Hi @rvitaliy, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@ghost ghost moved this from Merge in Progress to Recently Merged in Pull Requests Dashboard Aug 15, 2020
@rvitaliy rvitaliy deleted the feature/improve-autoload-php branch August 16, 2020 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests improvement Priority: P3 May be fixed according to the position in the backlog. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
Pull Requests Dashboard
  
Recently Merged
Development

Successfully merging this pull request may close these issues.

[Issue] improve and make more strict autoload.php
7 participants