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

[FEATURE] Allow overriding "web-dir" with environment variable #146

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

liayn
Copy link

@liayn liayn commented Jun 22, 2023

For some edge cases it might be useful to override, what is defined
in the root composer.json file for the web-dir, to allow
different web directories for different environments, without
changing the root composer.json

Resolves: #145

The Core uses TYPO3_PATH_ROOT for all paths related
to the document root.
The PackageArtifactBuilder (inside Core) uses the web-dir
to create the _assets symlinks.

Consequently, the web-dir must adhere to the TYPO3_PATH_ROOT
variable as well, in order to get a properly set up frontend.

Resolves: TYPO3#145
Copy link
Contributor

@helhum helhum left a comment

Choose a reason for hiding this comment

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

I'm sorry, this does not work out like this. Please provide more concrete examples of what you are trying to achieve in #145

@helhum
Copy link
Contributor

helhum commented Jun 22, 2023

I'll close this for now. Let's discuss #145 first

@helhum helhum closed this Jun 22, 2023
@helhum helhum reopened this Jun 26, 2023
For some edge cases it might be useful to override, what is defined
in the root composer.json file for the web-dir, to allow
different web directories for different environments, without
changing the root composer.json
@helhum helhum changed the title [BUGFIX] TYPO3_PATH_ROOT must override web-dir [FEATURE] Allow overriding "web-dir" with environment variable Jun 26, 2023
@helhum helhum marked this pull request as draft June 26, 2023 08:18
@liayn
Copy link
Author

liayn commented Jul 28, 2023

I backported this change to v3.1 now in my fork (but with using TYPO3_ROOT_PATH again) and it works brilliantly!

We should merge this for v4 and v5.
(I don't mind introducing a new env var, but we could simply stick with TYPO3_ROOT_PATH, it's the same anyway)

@helhum
Copy link
Contributor

helhum commented Sep 11, 2023

I backported this change to v3.1 now in my fork (but with using TYPO3_ROOT_PATH again) and it works brilliantly!

@liayn Thanks. I finally asked the core team for feedback. Pretty sure this will not end up in 3.1 though, as the impact of changing the web-dir is quite different here.

We should merge this for v4 and v5.

v4 is only a release candidate and will never be published as stable. So not going to be merged there either. So we're discussing putting that into v5.

(I don't mind introducing a new env var, but we could simply stick with TYPO3_ROOT_PATH, it's the same anyway)

Using an identical name for different things would cause more confusion than it would help imho. All TYPO3_*_PATH environment vars are exposed during runtime (and also influence runtime). What we are discussing here is influencing Composer build time.
Also TYPO3_ROOT_PATH is an absolute path, whereas web-dir and now TYPO3_COMPOSER_WEB_DIR is relative to composer root path.

@liayn
Copy link
Author

liayn commented Sep 11, 2023

Using an identical name for different things would cause more confusion than it would help imho. All TYPO3_*_PATH environment vars are exposed during runtime (and also influence runtime). What we are discussing here is influencing Composer build time.
Also TYPO3_ROOT_PATH is an absolute path, whereas web-dir and now TYPO3_COMPOSER_WEB_DIR is relative to composer root path.

Right, I simply set TYPO3_ROOT_PATH in the FPM/docker environment already to the correct absolute path, so that works of course.

As said above, I have absolutely no problem with a dedicated ENV var for this (build time) purpose, but it will/must influence the runtime variable TYPO3_ROOT_PATH anyways, otherwise things would be broken again.

@liayn liayn marked this pull request as ready for review September 11, 2023 11:19
@liayn
Copy link
Author

liayn commented Feb 20, 2024

@helhum Can we get this one finished?

@bmack
Copy link
Member

bmack commented Apr 12, 2024

@liayn can you elaborate a bit more? If I understand correctly, setting TYPO3_COMPOSER_WEB_DIR will mean that web-dir will not have any effect anymore... this just seems like a really weird proposal (just from reading through it). I mean, I saw your setup examples, but in my examples I just symlink and send additional headers from F5 to deal with a different folder structure when behind the LB.

@liayn
Copy link
Author

liayn commented Apr 12, 2024

@bmack Thanks for picking this up. I'll gladly explain.

We are not in the position to request changes to F5 (or whatever reverse proxy they are using rightnow)
And they are, in fact not necessary at all.

If I understand correctly, setting TYPO3_COMPOSER_WEB_DIR will mean that web-dir will not have any effect anymore...

No, this is absolutely not the case. The env var only changes the web-dir to a different folder, everything works like normal.
Helmut's request was to introduce a new ENV var, but I still believe this is not even necessary.

We have this setup running with a v11 instance now for a long time.
The only change I need is best explained with bfb7648

I'll gladly show you the instance and the setup in person as this thread here got quite lengthy over time.

@helhum
Copy link
Contributor

helhum commented Apr 14, 2024

@bmack Thanks for picking this up. I'll gladly explain.

We are not in the position to request changes to F5 (or whatever reverse proxy they are using rightnow) And they are, in fact not necessary at all.

Another alternative is manually creating a symlink to the public folder, that matches the desired structure.

Helmut's request was to introduce a new ENV var, but I still believe this is not even necessary.

The existing env var and the env var introduced here have very little in common.

The existing TYPO3_PATH_WEB is set as absolute path and is exposed for application runtime.
The proposed TYPO3_COMPOSER_WEB_DIR is meant to be a relative path (relative to folder of root composer.json) and is evaluated at Composer build time

In fact, they have so little in common, that I would find it very confusing to use the same env var name for it.

@bmack setting TYPO3_COMPOSER_WEB_DIR for Composer build time (e.g. TYPO3_COMPOSER_WEB_DIR=htdocs composer install), will override, what has been set in root composer.json. It leads to the same result as editing composer.json and putting 'htdocs' as web-dir

@helhum
Copy link
Contributor

helhum commented Apr 14, 2024

@bmack Thanks for picking this up. I'll gladly explain.

I asked for feedback in the core team months ago and a few days ago again and feedback was very reluctant.

My / our biggest concern still is, that people will misuse this somehow, where a more simple solution is possible. That said, I still did not fully understand, why any other option we discussed, did not work for you.

@liayn
Copy link
Author

liayn commented Apr 15, 2024

I asked for feedback in the core team months ago and a few days ago again and feedback was very reluctant.

Thanks for that. We have exactly 2 customers with a setup like this. The first one, we created quite some years back, I wasn't aware of this issue here, but we had access to the reverse proxy and could make some changes there to make it work.
The second one, which I am talking about here, we are not in the position to adjust things on the proxy. But I realized, that this is not necessary at all and that actually this is the way cleaner solution than fiddling with the proxy.

My / our biggest concern still is, that people will misuse this somehow, where a more simple solution is possible. That said, I still did not fully understand, why any other option we discussed, did not work for you.

The only "more simple solution" I have on my radar, is the suggestion to create the symlink. I honestly forgot what the problem was there, but I'll gladly give this a shot.


My whole goal of this patch is:

  • Make things easier for people with this kind of setup (without all the reverse-proxy setup hassle)
    But I have absolutely no problem, if you/others categorize this as edge case. I will keep using my fork.
  • The web-dir config in composer.json IMHO wrong at this place. It depends on the actual installation (and environment) and is not a property of the project itself. It's ok to define a "default" there, but it must be adjustable per installation. That's what this is supposed to achieve.

Explanation for @bmack how I came to my initial patch as can be seen here: bfb7648

In
https://github.com/TYPO3/CmsComposerInstallers/blob/main/res/php/autoload-include.tmpl.php#L15
we generate the code, that creates the TYPO3_PATH_WEB env var based on the web-dir config in composer.json.
That env var is used in core then.
My patch simply adds a check if TYPO3_PATH_WEB is already pre-defined in the environment and uses this as web-dir then, consequently ensuring that autoload-include.tmpl.php#L15 is re-initalizing TYPO3_PATH_WEB to the same path we already set in the environment. (and yes, it needs to be absolute)

@liayn
Copy link
Author

liayn commented May 8, 2024

Just a note that this kind of setup (public dir as a subfolder of docroot) is not a specialty of mine: Issue filed for a bug exactly in this usecase: https://forge.typo3.org/issues/103157

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

Successfully merging this pull request may close these issues.

The installer should adhere env TYPO3_PATH_ROOT
3 participants