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

Media URL on multisite with domain mapping #325

Closed
erralb opened this issue Aug 2, 2017 · 14 comments
Closed

Media URL on multisite with domain mapping #325

erralb opened this issue Aug 2, 2017 · 14 comments

Comments

@erralb
Copy link

erralb commented Aug 2, 2017

This might be a bug report.

I've been searching the web and roots forums but couldn't find anything, so if the answer already exists somewhere, please bear with me.

What is the current behavior?

I am using Bedrock for a multi site with domain mapping. Everything works fine, except for media files.

When I add a media file, instead of taking the current domain url, the media file takes the main domain url (aka the WP_HOME value from my .env file.)

So the file, instead of having a url like :

https://my_subsite_url.com/app/uploads/sites/3/2017/08/my_file.pdf

has a url like :

https://my_main_url.com/app/uploads/sites/3/2017/08/my_file.pdf

(The file IS accessible at https://my_subsite_url.com/app/uploads/sites/3/2017/08/my_file.pdf, but is not added by the wp editor correctly when editing content)

What is the expected or desired behavior?

That my file has a url like https://my_subsite_url.com/app/uploads/sites/3/2017/08/my_file.pdf when I add media in the backoffice)

I've had problems in the past with multisite and I would create different .env files and then load them accordingly to have my WP_CURRENT_SITE and WP_CURRENT_SITE_ID filles accordingly, but in this case I would like everything to be automatic from the site creation in the dashboard.

Please describe your local environment:

Bedrock version: 1.7.10

WordPress version: 4.8

PHP version: 7.1

OS: Debian

Where did the bug happen? Development or remote servers?

Remote server

Thanks

@retlehs
Copy link
Member

retlehs commented Nov 25, 2017

i don't think this is specific to bedrock, see also:

https://core.trac.wordpress.org/ticket/23221
https://core.trac.wordpress.org/ticket/23483

@retlehs retlehs closed this as completed Nov 25, 2017
@vladcosorg
Copy link

@retlehs

looks like it is specific to bedrock, on a vanilla installation it works just fine
the closed pull request #216 fixes the issue

The issue stems from the line

define('WP_CONTENT_URL', WP_HOME . CONTENT_DIR);

WP_HOME is a "static" value that does not change depending on currently opened network site but is taken from the .env file

define('WP_HOME', env('WP_HOME'));

Thus, all the assets urls that should have as base the current site domain are having as domain the primary website, which leads to a lot of issues, including the CORS warnings.

@retlehs
Copy link
Member

retlehs commented May 18, 2018

@chetzof do you have DOMAIN_CURRENT_SITE defined? what does your full config look like?

please open a topic on https://discourse.roots.io/ so we can do back & forth over there instead of on github

@vladcosorg
Copy link

@retlehs Yes, I do, posted the config here
https://discourse.roots.io/t/invalid-media-and-asset-url/12499

@Qoto
Copy link

Qoto commented Jun 7, 2018

Funny, how I still got this issue almost 3 years with 2 new installations and was still able to fix it with #216 maybe you can reconsider my pull request

greatislander pushed a commit to pressbooks/bedrock that referenced this issue Jan 29, 2019
Bumps [vlucas/phpdotenv](https://github.com/vlucas/phpdotenv) from 2.5.2 to 2.6.0.
<details>
<summary>Release notes</summary>

*Sourced from [vlucas/phpdotenv's releases](https://github.com/vlucas/phpdotenv/releases).*

> ## V2.6.0 (28/01/2019)
> We announce the immediate availability V2.6.0.
> 
> ### Bug Fixes
> 
> * Added missing throws doc ([roots#330](https://github-redirect.dependabot.com/vlucas/phpdotenv/issues/330))
> * Backport parser fixes from 3.3.0 ([roots#325](https://github-redirect.dependabot.com/vlucas/phpdotenv/issues/325))
</details>
<details>
<summary>Commits</summary>

- [`f3aae28`](vlucas/phpdotenv@f3aae28) Merge pull request [roots#325](https://github-redirect.dependabot.com/vlucas/phpdotenv/issues/325) from vlucas/parser-backport
- [`e918eac`](vlucas/phpdotenv@e918eac) Test both variants
- [`e53b2c3`](vlucas/phpdotenv@e53b2c3) Test parsing quoted slash
- [`5f0fbb8`](vlucas/phpdotenv@5f0fbb8) Fixed PHP 5.3 support
- [`2a0c63f`](vlucas/phpdotenv@2a0c63f) Backport new parser design, keeping BC
- [`cea7e2e`](vlucas/phpdotenv@cea7e2e) Added test for large variables ([roots#335](https://github-redirect.dependabot.com/vlucas/phpdotenv/issues/335))
- [`aa4be46`](vlucas/phpdotenv@aa4be46) Added missing throws doc ([roots#330](https://github-redirect.dependabot.com/vlucas/phpdotenv/issues/330))
- [`d7f715a`](vlucas/phpdotenv@d7f715a) Updated branch alias
- See full diff in [compare view](vlucas/phpdotenv@v2.5.2...v2.6.0)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=vlucas/phpdotenv&package-manager=composer&previous-version=2.5.2&new-version=2.6.0)](https://dependabot.com/compatibility-score.html?dependency-name=vlucas/phpdotenv&package-manager=composer&previous-version=2.5.2&new-version=2.6.0)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
@Memolition
Copy link

Memolition commented Jun 27, 2019

Hello there, I've hit this issue while working on a WP Network installation with bedrock and it was only after having it solved as a result of a huge amount of wasted time that I found about PR #216.

PR #216 was closed with an invalid reason and even if multisite works "fine" in most TESTING cases without this fix, it really doesn't for a production deploy. Having a 'blog' pull its own resources from another domain just doesn't work. Unlike Issue #224 I don't see how this fix could break any functionality or produce any side effects.

There is an "almost-required" plugin needed for networks/multisites to work if using bedrock (see roots/multisite-url-fixer) and I thought of proposing this solution there, but I don't think url-fixer can modify the configuration files since clearly as stated on a closed issue, it is out of its scope

By the way, even if this may be a WP-Core issue, I think that having the chance (and the PR ready) to fix an issue like this and not taking it totally defeats the purpose of building a boilerplate meant to "ease" the setup and management process of WP.

i don't think this is specific to bedrock, see also:

https://core.trac.wordpress.org/ticket/23221
https://core.trac.wordpress.org/ticket/23483

@retlehs
Copy link
Member

retlehs commented Jun 27, 2019

Hello there, I've hit this issue while working on a WP Network installation with bedrock and it was only after having it solved as a result of a huge amount of wasted time that I found about PR #216.

“huge amount of wasted time” … bud, do you have any idea how much wasted time has been spent by volunteers on this project reading issues and providing support? honestly i stopped reading after that because the sense of entitlement some of y’all have in open source is 🙈

@str
Copy link

str commented Jun 27, 2019

@retlehs, I'm sorry you feel that way. I hope you and more people could see the technical facts over what people feel and see the fixes here as a good technical solution.

If anyone else sees @chetzof changes as solution, please upvote it.

Thank you.

@Memolition
Copy link

Memolition commented Jun 27, 2019

Hello there, I've hit this issue while working on a WP Network installation with bedrock and it was only after having it solved as a result of a huge amount of wasted time that I found about PR #216.

“huge amount of wasted time” … bud, do you have any idea how much wasted time has been spent by volunteers on this project reading issues and providing support? honestly i stopped reading after that because the sense of entitlement some of y’all have in open source is see_no_evil

Sure can you be that I have an exact idea of how much time can been invested (not wasted) by volunteers in a project like this to produce some amazing work.

That's exactly why I mentioned that you already had the solution in your hands and that sure is wasted
time, and not because @chetzof didn't do it right.

Also isn't "open-source" all about community contributions? The contribution has already been made, I'm supporting it since it is a good one. Or are you suggesting that I should do a PR of my own to do the exact same thing PR #216 does and therefore "wasting" more volunteered time?

@swalkinshaw
Copy link
Member

swalkinshaw commented Jun 27, 2019

The solution proposed in #216 is a much more fundamental change. Yes it may solve this particular issue, but it changes WP_CONTENT_URL from being statically defined to being dynamic based on $_SERVER['HTTP_HOST'].

This could have large effects outside of multisite installs as well. So if we want a resolution to this, we need to have a discussion about this change in general and how it fits in with Bedrock's philosophy and configuration system.

Anyone can open a PR/issue about this generic change. If you can provide any details on the effects/behaviour of this change, that would help us out 👍

@Memolition
Copy link

@swalkinshaw Could it be good to have a "switch" to enable it from .env whenever it is needed for ?multisites

@swalkinshaw
Copy link
Member

Maybe, but ideally if this change is just better all around then we can use it without any condition. And I'd rather try and figure that out first if possible.

@JulienMelissas
Copy link
Contributor

JulienMelissas commented Jun 27, 2019

@Memolition -- I want to first thank you for your detail and referencing in your first post. I also want to say that any time you decide to post about your frustrations, please remember that these projects are maintained by people who have no guarantee of being paid and are often frustrated by problems with their own work!

I want to respond with a few points, and a suggested "solution", I would appreciate your feedback, or anyone else's:

First of all, I closed the issue over a while ago at roots/multisite-url-fixer#5 because it was indeed out of scope. Maybe the naming of the plugin is misleading (I am proposing a README change in the project here: roots/multisite-url-fixer#8), but it exists only to fix some core WP issues.

That brings me to my second point, which is to remind everyone that Bedrock is a boilerplate, as with almost all of the Roots projects. This means that we do not intend to modify or break default WP behavior as much as possible, especially when messing with constants such as WP_CONTENT_URL, since it could have bigger unexpected effects on other installations. We don't want to cause a load of "wasted time" because someone expected default WP behavior. We do expect you to make modifications based on the needs of your project.

Speaking of default WP... the current behavior, in my understanding, is actually not a bug. In my case (I maintain two separate SaaS-like WordPress Multisites), I actually appreciate the URLs for my assets and media to come from the multisite's main domain, it provides me with a lot of flexibility. I just did some research (keep in mind I hate reading trac tickets) and I couldn't find exactly why the decision to do it like that was made, but that's how it is.

That said, I also understand why this could be an issue for many people running multisites on Bedrock and that there is a fix for it (#216) that we closed, so I think that some improved documentation could be in order. There is a section on multisite on the main Bedrock install page, and I think we would be open to either linking to that PR, a blog post, or even maybe including the suggestions directly on the page. Would you be interested in contributing?

Open to any additional thoughts, but please keep all of the above in mind!

@Memolition
Copy link

@JulienMelissas Your solution on expanding the multisite section sounds great since currently most of the references to something going wrong with /wp link to that section now that the best solution to that directory issues seems to be roots/multisyte-url-fixer. I thought of proposing the same thing, but didn't remember about that section and there is not much detail on "Bedrock Multisites/Netowk" other than on roots/trellis Multisite page.

Predicting the whole behaviour of WP_CONTENT_URL or the outcome of fiddling with it, I think would be pointless since it is subject to any changes to the WP functionality, just as @swalkinshaw mentioned before. But other than "Enabling and allowing the toggle of these proposed settings in the mentioned PR" (also the workaround from roots/multisite-url-fixer#3) or just "Documenting the workaround", I don't see many other possible solutions.

Now, I might have sounded a little compulsive on my redaction, however my point is: It took me long and whatever else but I already solved this issue for me and regardless of what happens on this repo I already have a fork and applied this fix, because it suits my needs and there seems to be no side-effects whatsoever to the functionality I need.

However quoting Ben on the what the "Entitlements of Open Source" should be, hiding my solution or not even trying to bring it up as if nobody else is using these same package doesn't form part of My philosophy of Community and Open-source.

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

No branches or pull requests

7 participants