Introduced pub/ structure for webserver home folder mapping

[speedupmate]

introducing pub/{n}_website folder structure to serve public assets while hiding away magento_root from public areas.

Description (*)

• lessen attack surface by only serving what is public
• allows version controlled management of any sites (including test and staging sites) configuration (if different)
• separates public folder from core code so core can be updated without needing to merge .htaccess , index.php changes etc as those would be separated

Fixed Issues (if relevant)

1. Fixes introduce a /pub/ directory analog to Magento2 #571

Manual testing scenarios (*)

1. set your webserver config to point to a new home folder under pub/{x}
2. alternatively symlink your webserver home folder to desired location under pub/{x}

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[colinmollenhour]

Ahh, I've always wished the repo root was not the web root. That's a clever way to avoid renaming every file in the repo. I like it! 👍

[fballiano]

Questions:

• Is this compatible with the installation via composer?
• are those directories symlinks?
• I think we need to update the .htaccess in this PR cause the main one was changed a lot

[speedupmate]

Questions:

• Is this compatible with the installation via composer?
• are those directories symlinks?
• I think we need to update the .htaccess in this PR cause the main one was changed a lot

• I have no idea , it probably is compatible since composer only deals with filesystem
• yes those are symlinks , can be apache nginx directives/aliases also but thats more complicated
• actually .htaccess can also be a symlink by default and whoever needs to edit this can swap it out for real one

[fballiano]

this PR is tricky nowadays, htaccess and index.php are very different and we've to rebase it to main or next branch :-\

[addison74]

I think this PR can be closed. Moving some directories does not bring any improvement, on the contrary, it complicates a structure that has been working for more than 15 years. In addition, there are files that will create a conflict with what already exists in the repository.

[colinmollenhour]

@addison74

... it complicates a structure that has been working for more than 15 years. ...

I think this PR has it's merits. Every other web project I've ever seen has a separate pub directory so that every project file is not in the root. M2, Laravel, Rails, etc.. I've never seen another one that relies on .htaccess files sprinkled around to "secure" everything.

You know what happens if you are migrating files from one directory to another and you forget to copy or delete a .htaccess file? Boom, your files are wide open! Ask me how I know.. :(

Also every time someone adds a new file to the repo it is a risk that it isn't protected.. Example:

https://demo.openmage.org/LICENSE.html

I would actually be in favor of moving index.php into pub for the next major version and making this mandatory. It's worth the effort to reorganize the code to expose only what you want, but probably very few do. In my opinion the project should set an example for a good deployment, not encourage bad practice. Yes, I know, kinda late to be tackling this now... :D

[colinmollenhour]

I addressed the duplicate files in the latest commit (symlinks and require) and also updated the dev/openmage environment to use the new pub directory and verified that this works and does indeed solve the document root issue:

[addison74]

@colinmollenhour - The argument you offered is enough to find PR utility, but I have never encountered the mentioned problems and I hope this never happens. PR still needs polish and documentation. It was proposed in main branch.

[colinmollenhour]

@addison74

PR still needs polish and documentation.

Please provide specifics regarding polish. Where do you propose this should be documented?

It was proposed in main branch.

As implemented currently this actually doesn't break BC in any way so I think this could be added to "main". If there is support for breaking BC on purpose then it should definitely be in "next" but I think that will be too controversial..

I see @fballiano just merged while I was typing. 😄 I'm happy to add docs for this, just let me know where. It can be improved upon if any issues are found but I currently do not know of any issues with it.

[fballiano]

when my question was answered I was ok with that, we can add docs in any moment ;-)

[fballiano]

and i was also sure it's not BC

[fballiano]

sadly this PR doesn't work when the project is installed via composer if deploy strategy is "copy"

can we do something about that?

[speedupmate]

@fballiano you can pro provide a post script that inspects the copy strategy and fixes symlinks if those fail to be copied and need to be recreated

[addison74]

Shouldn't we include steps how to use this feature in OM?

Also, is there any solution when OM is used together with composer as @fballiano reported above?

[fballiano]

Since the "pub" folder is totally incompatible with composer installation, which is for use the best way to install/upgrade/manage OpenMage, I'm not even sure this folder should be in our repo, because it's simply broken now (with composer)

[addison74]

Shall we revert this PR?

[fballiano]

This PR has some other things not really related to the "pub/" structure so I'd not revert but I'd evaluate the removal of just the pub/ folder, converting it to a documentation on how to create it...

[colinmollenhour]

Perhaps the composer modules which deploy the Magento repo could be fixed to support copying the symlinks? Which one did you use, @fballiano ?

[fballiano]

@colinmollenhour I use "aydin-hassan/magento-core-composer-installer", both for php7 and php8 (depending on the project)

[fballiano]

I think we could just introduce an exclude for the pub directoy

         "magento-core-deploy" : {
            "excludes": [ 
                "pub"
            ]
         }

the feature is still good and it's good to have it in the vendor, but deploying it is not very usefull cause it will have to be customized anyway (per website) so if we don't deploy it at all we've no problem.

[ma4nn]

perhaps the magento-core-deploy config should be mentioned somewhere in the installation notes, right?

[kiatng]

Ref

magento-lts/errors/processor.php

Lines 113 to 116 in 611bbe7

	$this->_prepareConfig();
	if (isset($_SERVER['MAGE_ERRORS_SKIN']) || isset($_GET['skin'])) {
	$this->_setSkin($_SERVER['MAGE_ERRORS_SKIN'] ?? $_GET['skin']);
	}

Line 113 $this->_prepareConfig(); above load the skin defined in errors/local.xml, which will be overwritten in line 115 $this->_setSkin($_SERVER['MAGE_ERRORS_SKIN'] ?? $_GET['skin']);.

For anyone who is wondering why the error page is rendering the default skin and not the expected skin, remove the following lines:

magento-lts/pub/default/index.php

Lines 20 to 23 in 611bbe7

	/* Errors pages skin */
	if (empty($_SERVER['MAGE_ERRORS_SKIN'])) {
	$_SERVER['MAGE_ERRORS_SKIN'] = 'default';
	}

in your pub/{store_or_website_name}/index.php.

[sreichel]

@kiatng is this still valid https://github.com/OpenMage/magento-lts/blob/main/docs/ERROR_PAGE.md ?

[kiatng]

ERROR_PAGE.md is valid if $_SERVER['MAGE_ERRORS_SKIN'] is not set. I'm not sure why this PR introduced the new param MAGE_ERRORS_SKIN, it seems unnecessary to me.