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

Issues with switching to public as the default web root #3363

Closed
m-vo opened this issue Aug 22, 2021 · 8 comments · Fixed by #3535
Closed

Issues with switching to public as the default web root #3363

m-vo opened this issue Aug 22, 2021 · 8 comments · Fixed by #3535
Labels
Milestone

Comments

@m-vo
Copy link
Member

m-vo commented Aug 22, 2021

Affected version(s)

Contao 4.12?

Description

Detecting the use of web as public directory by checking if the resource exists on the filesystem might lead to unwanted behavior, see #3084 (comment).

I noticed the problem when creating a new project from a template/skeleton (now with 4.12):

  • the project assumes web as the web root
  • web, however, does not exist initially → public gets created on install
  • webpack is run and outputs stuff into the now created web directory
  • the symfony server is started and picks up public as the web root
  • the cache is cleared after some work, now web exists and things break*)

*) because the contao.web_dir config setting now differs from the actual web root

The same thing will happen when deploying by copying a bare minimum and installing, i.e. without explicitly creating the web directory. There the website will stop working as no entry points will be at the expected location.

IMHO we should neither rely on the existence nor absence of the web directory but only on the config. I've also seen projects using web as their js/layout root. 🙈

What about this strategy instead?

  • use extra.public-dir in the app's composer.json to set the public dir like Symfony does
  • default config to web unless configured otherwise via composer.json (or config)
  • ship the switch to public via the 4.12 managed edition composer.json, so that only new projects are affected

We could then ultimately change the default in Contao 5 and remove the composer.json config key again (as a BC break).

@m-vo m-vo added this to the 4.12 milestone Aug 22, 2021
@m-vo m-vo changed the title Issues with switching to public as the default a web root Issues with switching to public as the default web root Aug 22, 2021
@leofeyer leofeyer added the bug label Aug 23, 2021
@leofeyer
Copy link
Member

I don't think we should do any of this. Keep in mind that the logic only applies to the default value, therefore all of the problems mentioned above can be solved by adding contao.web_dir to your config.yml file.

@m-vo
Copy link
Member Author

m-vo commented Aug 23, 2021

So if I'm installing Contao in a Symfony environment where I set my web root via the composer.json I need to add a config entry. Same if I am using a new 4.12 with public and later on I'll add a web folder - not knowing this would do any harm. I don't like that magic at all.

@m-vo
Copy link
Member Author

m-vo commented Aug 23, 2021

related #3370

@leofeyer
Copy link
Member

So if I'm installing Contao in a Symfony environment where I set my web root via the composer.json I need to add a config entry.

Yes. We have decided against using extra.public-dir from the composer.json file, which is why we have added a separate contao.web_dir parameter in the first place. I don't recall the exact reasons but I think it was because there is no way to add custom settings to the composer.json file yet.

Same if I am using a new 4.12 with public and later on I'll add a web folder - not knowing this would do any harm. I don't like that magic at all.

That is indeed something that we could reconsider, although I don't think many users will run into the issue. Any ideas how to improve it?

@m-vo
Copy link
Member Author

m-vo commented Aug 24, 2021

Yes. We have decided against using extra.public-dir from the composer.json file, which is why we have added a separate contao.web_dir parameter in the first place. I don't recall the exact reasons but I think it was because there is no way to add custom settings to the composer.json file yet.

It wouldn't do any harm to support it now, would it? I'd read it once when parsing the config and only use the value if extra.public-dir is set and contao.web_dir is not set.

Any ideas how to improve it?

Only trust the config key: the usage would then be consistent at least and would be easier to debug. The auto-detection could still be in there when parsing the config value.

But like I said, I wouldn't try to detect folders at all. 🙈

@leofeyer leofeyer added the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Aug 24, 2021
@aschempp
Copy link
Member

#3084 (comment) 🤷

@leofeyer
Copy link
Member

As discussed in the Contao call on September 30th, we want to set contao.web_dir from extra.public-dir in the composer.json file and deprecate the parameter in the configuration tree.

@leofeyer leofeyer modified the milestones: 4.12, 4.13 Sep 30, 2021
@leofeyer leofeyer added feature and removed bug up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. labels Sep 30, 2021
@m-vo
Copy link
Member Author

m-vo commented Oct 1, 2021

see #3535

@m-vo m-vo closed this as completed Oct 1, 2021
leofeyer pushed a commit that referenced this issue Oct 15, 2021
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes #3363
| Docs PR or issue | -

#3363 (comment)

> As discussed in the Contao call on September 30th, we want to set contao.web_dir from extra.public-dir in the composer.json file and deprecate the parameter in the configuration tree.

Commits
-------

1a199c0 read extra.public-dir from composer.json + deprecate contao.web-dir container config
253010a fix phpstan + tests
0b9dd32 restore unrelated changes
73745cd Merge branch '4.x' into feature/composer-web-dir
47fc78c CS
81ce2db CS
leofeyer pushed a commit to contao/core-bundle that referenced this issue Oct 15, 2021
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes #3363
| Docs PR or issue | -

contao/contao#3363 (comment)

> As discussed in the Contao call on September 30th, we want to set contao.web_dir from extra.public-dir in the composer.json file and deprecate the parameter in the configuration tree.

Commits
-------

1a199c04 read extra.public-dir from composer.json + deprecate contao.web-dir container config
253010a8 fix phpstan + tests
0b9dd329 restore unrelated changes
73745cd2 Merge branch '4.x' into feature/composer-web-dir
47fc78c2 CS
81ce2db2 CS
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants