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

Rename the web folder to public #3084

Merged
merged 5 commits into from Jun 18, 2021
Merged

Conversation

leofeyer
Copy link
Member

@leofeyer leofeyer commented Jun 14, 2021

Q A
Fixed issues Implements #1088
Docs PR or issue contao/docs#776

This PR renames the web folder to public as discussed here: #1088 (comment)

@contao/developers How do we want to implement backwards compatibility? I see two options:

  1. Add a check in the Configuration class: If contao.web_dir is public and there is no public folder but a web folder, change the value to web automatically.

  2. Make the GenerateSymlinkCommand generate a symlink from public to web if a web folder exists.

I am in favor of solution no. 1, because I find having two different public folders confusing. WDYT?

@leofeyer leofeyer added this to the 4.12 milestone Jun 14, 2021
@leofeyer leofeyer requested a review from a team June 14, 2021 10:29
@leofeyer leofeyer self-assigned this Jun 14, 2021
@leofeyer leofeyer linked an issue Jun 14, 2021 that may be closed by this pull request
@fritzmg
Copy link
Contributor

fritzmg commented Jun 14, 2021

Don't we need to solve #2461 first?

@leofeyer
Copy link
Member Author

No:

As discussed in Mumble on March 11, it should work in Contao 4.9 if you set the web directory in both the composer.json and the app configuration. If this is not the case, we should fix it.

It seems to work though.

@leofeyer leofeyer linked an issue Jun 14, 2021 that may be closed by this pull request
@aschempp
Copy link
Member

Maybe we should change this default (once and for all) in Contao 5 only? That would probably solve a lot of potential headaches (including the Contao Manager).

@leofeyer
Copy link
Member Author

We have already discussed this (see #1088 (comment)).

@leofeyer
Copy link
Member Author

This PR is ready to merge. I have thoroughly tested the changes and made sure that the installation commands fall back to web if the directory exist. It therefore does not matter when the Contao Manager is adjusted to use public.

@leofeyer leofeyer requested review from ausi and fritzmg June 18, 2021 09:17
@leofeyer leofeyer merged commit 51d174d into contao:4.x Jun 18, 2021
@leofeyer leofeyer deleted the feature/web-to-public branch June 18, 2021 16:15
@aschempp
Copy link
Member

This PR is ready to merge. I have thoroughly tested the changes and made sure that the installation commands fall back to web if the directory exist. It therefore does not matter when the Contao Manager is adjusted to use public.

did you also test this with the Contao Manager? e.g. if it is possible to manage an installation that uses the public dir?

@leofeyer
Copy link
Member Author

The Contao Manager does not yet support anything but web, because there are hardcoded references to the name e.g. here: https://github.com/contao/contao-manager/blob/8d86a863d585d5d202c9dec85cb29d977f896ab9/api/ApiKernel.php#L261-L262 We will have to adjust this at some point in the future.

@aschempp
Copy link
Member

Does that mean you cannot manage Contao 4.12 with the Contao Manager?

@leofeyer
Copy link
Member Author

Of course not. 😄 You just have to use web instead of public.

@fritzmg
Copy link
Contributor

fritzmg commented Jun 22, 2021

A problem would arise if you install the Contao Manager after installing Contao via composer create-project. But other than that it should be fine, since the Contao Manager requires you to create a web directory.

Comment on lines +103 to +109
if (null === $this->webDir) {
if ((new Filesystem())->exists($this->projectDir.'/web')) {
$this->webDir = 'web'; // backwards compatibility
} else {
$this->webDir = 'public';
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Now testing this, I'm wondering why we do not just rely on the config value here? IMHO the config should be the single point of truth once configured.

@m-vo
Copy link
Member

m-vo commented Aug 21, 2021

I fear there is still something fishy with this change… 🐟

In an 4.12 installation where I did not configure anything, public was used as a web root but the contao.web_dir config key/parameter was still pointing to web. This for instance broke the back end as the combiner was getting the wrong directory until I explicitly configured it to %kernel.project_dir%/public.

I think there is another problem with detecting the old default by checking if the web directory exists: if you are using a deployment strategy that runs contao-setup on installation, no folders might exist beforehand. Your web root will now change. 😢

(https://contao.slack.com/archives/CK4J0KNDB/p1629567427484400)

@leofeyer
Copy link
Member Author

It does work like a charm on contao.org, therefore I suggest you debug the problem and open a new issue if it turns out to be a Contao problem.

@fritzmg
Copy link
Contributor

fritzmg commented Aug 22, 2021

I am also unable to confirm the problem. After running composer create-project contao/managed-edition . 4.12.* a public folder was created and %contao.web_dir% properly points to that directory.

@fritzmg
Copy link
Contributor

fritzmg commented Aug 22, 2021

Ah I see what you mean now with deployments. That is indeed a problem, since this will break the live site after a deployment (because the deployment would not fail in that case). For our deployments it's not a problem since we usually have versioned files in the web/ directory anyway (like .htaccess files or other public resources). But if you don't need to version anything in web/ this will for sure break those deployments. Not sure how to solve this though?

@m-vo
Copy link
Member

m-vo commented Aug 22, 2021

Not sure how to solve this though?

I think we can't auto detect the old state. Instead we IMHO should use a config key in the composer.json file that is read and that sets the config. Then we could safely introduce the change in a new version of contao/managed-edition without affecting old installations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants