Add an option for setup:config:set to define the document root

[AntonEvers]

Description (*)

Add bin/magento setup:config:set --document-root-is-pub
I think this is needed in order to raise awareness for https://devdocs.magento.com/guides/v2.3/install-gde/tutorials/change-docroot-to-pub.html section 3, and to simplify automating this setting in deployment scripts.

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

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] Add an option for setup:config:set to define the document root #29523: Add an option for setup:config:set to define the document root

[m2-assistant]

Hi @AntonEvers. 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.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[AntonEvers]

@magento run all tests

[ihor-sviziev]

@AntonEvers good idea!
Could you cover this new command with any kind of test? I think unit will be ok there.

Based on the description i have few questions:

1. What if we'll run just bin/magento setup:config:set --document-root-is-pub? Will it set to "true" or it will ask for setting true / false?
2. What if we'll run just bin/magento setup:config:set --document-root-is-pub true? Will it set to "true"?
3. What if we'll run bin/magento setup:config:set --document-root-is-pub false? Will it set it as "false"?

Would be nice to have more clear description for the command option

[AntonEvers]

@AntonEvers good idea!
Could you cover this new command with any kind of test? I think unit will be ok there.

Based on the description i have few questions:

1. What if we'll run just bin/magento setup:config:set --document-root-is-pub? Will it set to "true" or it will ask for setting true / false?
2. What if we'll run just bin/magento setup:config:set --document-root-is-pub true? Will it set to "true"?
3. What if we'll run bin/magento setup:config:set --document-root-is-pub false? Will it set it as "false"?

Would be nice to have more clear description for the command option

Hi @ihor-sviziev
The command option currently works similar to --allow-parallel-generation, --skip-db-validation, --db-ssl-verify, --cleanup-database and --use-sample-data.
It is a flag, rather than an option and it does not allow a value. bin/magento setup:config:set --document-root-is-pub would add the following to env.php:

'directories' => [
    'document_root_is_pub' => true
]

I will add some unit testing. Please let me know if the command should behave more like --use-secure=USE-SECURE instead, to allow setting it both to true or false.

[AntonEvers]

I changed the command to a boolean select option.
During interactive installation it will now asK:

Is the webserver's document root set to ./pub/?
  [0] no
  [1] yes

It accepts inputs: yes, no, 1, 0, true, false.

[AntonEvers]

Now I have set the default value to false, so that the key is always present in env.php. This should make developers more aware of the fact that this option exists.

[ihor-sviziev]

From first look - looks good!
@magento run all tests

[ihor-sviziev]

Hi @AntonEvers ,
Could you fix static tests? Everything else looks good

[AntonEvers]

@magento run Static Tests

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

Looks like that’s last part of requests changes, sorry for not requesting them earlier.

[AntonEvers]

No problem at all @ihor-sviziev!

[AntonEvers]

@magento run all tests

[ihor-sviziev]

✔ Approved.

Failing tests looks not related to changes form this PR.

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7889 has been created to process this Pull Request

[gabrieldagama]

@magento run all tests

[gabrieldagama]

@magento run all tests

[sidolov]

@magento run all tests

[ihor-sviziev]

@gabrieldagama @sidolov is there any reasons why this PR isn't getting merged?

[gabrieldagama]

@ihor-sviziev We are having some issues with its tests that are being looked at internally. Just for reference, the internal ticket is https://jira.corp.magento.com/browse/MTS-1593

[ihor-sviziev]

Hi @AntonEvers,
According to the recent changes in 2.4-develop - the support of non-pub was removed and now Magento always uses the pub directory (finally👍) .
640cad5#diff-2323d6157444b3cf24d4d76635f27185a4e6ad979b0ec2af53e6592d3ca465d5R13

Due to this change I think we could close this PR as not relevant anymore.
What do you thing?

[m2-assistant]

Hi @AntonEvers, 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.

[hostep]

@ihor-sviziev: nice find!

But I really don't understand how they can make such a major change in a minor release? This change will probably be scheduled for Magento 2.4.2, even though it should be for 2.5.0. This might break a lot of installations.

I'm not saying it's a bad thing, it should have happened much earlier, but big changes like these really shouldn't be done in a minor release.

(sorry to interrupt here in this PR)

[ihor-sviziev]

@sivaschenko @sidolov could you provide more info why this commit was merged to 2.4-develop as it's introducing backward incompatible changes? 640cad5#diff-2323d6157444b3cf24d4d76635f27185a4e6ad979b0ec2af53e6592d3ca465d5R13

[shiftedreality]

@hostep we updated Apache config to use pub directory. Since Apache reads configs on-fly, we didn't notice any issue having it updated.

This solves a few issues:

• Deprecates usage of \Magento\Config\Model\Config\Reader\Source\Deployed\DocumentRoot. This class was added but not used in the places where it should.
• Standardizes pub directory paths all over magento without class above
• Standartazise both Apache and Nginx entrypoints

Without this change, we saw a lot of issues while adding RemoteStorage functionality and saw possible issues for developers who work with FS API.

Could you provide any possible risks which we might miss?

[hostep]

@shiftedreality: I understand why this is a good improvement, it really is, I and many others have been wanting it even 2 years before Magento 2.0.0 was launched.

But where I'm seeing problems is that certain users now will be surprised when they perform a minor update and notice they have to change their document root and somehow can't. I'm pretty sure that there are (cheap) hosting companies around which don't allow you to change the document root and make it very difficult to host magento from inside the pub directory. Which is a shame and people shouldn't use these sort of hosting companies, but yes, a lot of people want cheap hosting... So I think it only makes sense to introduce such a big change in a major release like 2.5.0 to avoid breaking shops just because they made a wrong decision in choosing a certain hosting company.
If that's really not possible and it needs to be shipped in 2.4.x, then the release notes will have to add a very big warning on top (and not hidden in the hundreds of changes) so that users are aware of this change.

[shiftedreality]

@hostep my understanding is those Customers have an Apache which reads .htaccess file. So, it should be supported fine, since the file is updated. Isn't this standard case for Apache on hosting companies?

[hostep]

The .htaccess file in the Magento root is deleted, so I'm not sure what you are saying?
Also, you can not change the document root from a .htaccess file as far as I'm aware.

[shiftedreality]

It exists. Just serves from pub: https://github.com/magento/magento2/blob/2.4-develop/.htaccess

[hostep]

Aaaaaah ok, the github interface tricked me, I assumed it was removed when I saw all those red blocks 😄
Okay, thanks for the explanation, those new lines in there will probably solve it (untested), sorry about the noise! 🙂

[shiftedreality]

Sure, thank you for paying so much attention and passion for Magento!

[ihor-sviziev]

@shiftedreality thx for good explanation!