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

Allow setting HTTP_HOST directly via --uri option #195

Closed
quicksketch opened this issue Aug 1, 2022 · 15 comments · Fixed by #276
Closed

Allow setting HTTP_HOST directly via --uri option #195

quicksketch opened this issue Aug 1, 2022 · 15 comments · Fixed by #276
Assignees

Comments

@quicksketch
Copy link
Member

I'm using bee together with ddev and finding that bee can't figure out what the hostname should be.

I've set up my ddev command as follows:

/usr/local/bin/bee/bee.php --root=/var/www/html/$DDEV_DOCROOT $@

When using a ddev command like ddev bee uli, I get back the output:

Use the following link to login as 'admin':
http://web/user/reset/1/1659380860/sA-ge2y4h7gkLsH4TEvAOSxGGeE_OlQaz4qBWRsccy0/login

The problem is bee doesn't know the URI of my site and it uses the hostname from the container. I can "fix" this by setting a $base_url in settings.php, but I would find it preferable if I could set the URI as an option.

I tried using --site=$DDEV_PRIMARY_URL, but the --site option is intended to select a multisite, and does not do much of anything when used on a single-site installation.

Proposed solution:

@quicksketch
Copy link
Member Author

I filed a PR at #196 that adds the option and documentation. I did not add any validation of the new option; do we validate other global options somewhere?

@yorkshire-pudding
Copy link
Collaborator

@quicksketch
I've confirmed that this issue also affects Lando.
I've tested in Lando (using the bee lando recipe) and while it does return the correct link, it also produces quite a few error messages:

user@machine-name:~/projects/backdrop/bee-195-uri/bee$ lando bee --root=backdrop --uri=https://bee.lndo.site uli
Undefined index: port
 /app/includes/miscellaneous.inc:54

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/settings.php:168

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/settings.php:169

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/settings.php:177

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/settings.php:184

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/core/includes/bootstrap.inc:979

session_name(): Cannot change session name when headers already sent
 /app/backdrop/core/includes/bootstrap.inc:991

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/core/includes/bootstrap.inc:745

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/core/includes/bootstrap.inc:746

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/core/includes/bootstrap.inc:747

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/core/includes/bootstrap.inc:750

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/core/includes/bootstrap.inc:752

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/core/includes/bootstrap.inc:979

session_name(): Cannot change session name when headers already sent
 /app/backdrop/core/includes/bootstrap.inc:991

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/settings.php:168

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/settings.php:169

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/settings.php:177

ini_set(): Headers already sent. You cannot change the session module's ini settings at this time
 /app/backdrop/settings.php:184

Use the following link to login as 'admin':
https://bee.lndo.site/user/reset/1/1660902216/yRusDxT-Ckm60HC6slREVXp0_wCLdzaAVMynylZT_lM/login

Just a thought. I appreciate that drush uses --uri and maybe that is the right thing to do for continuity, but would --url say the same thing with a more familiar term? I appreciate that bee users are not site builder novices necessarily, but just wondering about clear terminology, if they mean the same thing. In other places we use aliases (that allow multiple terms for the same command), but I don't think it's possible in global options - @BWPanda - is it? I don't have a problem with uri, and continuity with drush may be more important than user friendly terms in this instance.

@yorkshire-pudding
Copy link
Collaborator

@quicksketch - any thoughts about the errors in the comment above?

@yorkshire-pudding
Copy link
Collaborator

@quicksketch - I've found the cause of the issues in the comment above and suggested a change that fixes the error messages. Please also see my comments above regarding URL vs URI.

@yorkshire-pudding
Copy link
Collaborator

@quicksketch - please see the suggestion.

Having looked further at the documentation for drush, I think using --uri rather than --url may well confuse matters:

For multisite installations, use the --uri option to target a particular site. If you are outside the Drupal web root, you might need to use the --root, --uri or other command line options just for Drush to work.

$ drush --uri=http://example.com pm-updatecode

In bee we use --site=example.com / --site=example_com (latter is folder name) to refer to multi-sites so I think it would be best to have a clear break from drush on this option and go with --url. As mentioned previously, this would align with terminology in settings.php (i.e. $base_url).

Thanks

@yorkshire-pudding
Copy link
Collaborator

Given that the lando testing environment defines urls for the test sites, it would probably also make sense to have a test for this options, probably using the user-login/uli function.

@yorkshire-pudding
Copy link
Collaborator

Agreed with @quicksketch on 25th March that I can take over this PR to do the suggested changes.

@yorkshire-pudding yorkshire-pudding added pr: needs review pr: needs testing A pull request has been linked to the issue and requires testing. and removed tests pr: needs work labels Mar 27, 2023
@yorkshire-pudding
Copy link
Collaborator

A new PR has been prepared against this issue:

  • I have made the option be called base-url for clarity;
  • I have fixed some issues that were causing tests to fail
  • I have added this option into one of the tests for user-login/uli command

This is largely based on the work of @quicksketch

@ghost
Copy link

ghost commented Mar 27, 2023

Are there use-cases for this new option outside of the user-login command? If not, perhaps this should just be an option on that command, and not a global one...?

@yorkshire-pudding
Copy link
Collaborator

I cannot think of any others though it's not beyond the realms of possibility that a custom command run using eval, php-script, the API could need it. I did wonder about this, but was aiming to get over the line, just fixing the deficiencies in the original PR and as it is optional it doesn't affect anyone if they don't need it. I'm not averse to moving this; it would certainly make the documentation aspect more cohesive, but I can also see that leaving it in global has some future-proofing benefit.

From your greater knowledge of core, are there any functional areas that are more prone to problems if Backdrop can't work out $base_url? If so, and there is scope that something in that area could be invoked either by contrib, custom or even future commands (e.g. it's possible that if we do implement aliases that that could need the base url to be set).

My preference is to leave in global for future-proofing reasons.

@ghost
Copy link

ghost commented Mar 28, 2023

My preference is to leave in global for future-proofing reasons.

Ok. I've left some comments/suggestions on the PR.

@ghost ghost added pr: needs work and removed pr: needs review pr: needs testing A pull request has been linked to the issue and requires testing. labels Mar 28, 2023
@yorkshire-pudding
Copy link
Collaborator

@BWPanda - I've addressed the points in your comments.

@ghost
Copy link

ghost commented Mar 29, 2023

Thanks @yorkshire-pudding, I understand the global variable part properly now. Requested a few small changes, then should be good to go (I won't worry about changing the issue status).

@yorkshire-pudding
Copy link
Collaborator

Thanks @BWPanda - changes completed

@ghost ghost closed this as completed in #276 Mar 29, 2023
ghost pushed a commit that referenced this issue Mar 29, 2023
@ghost
Copy link

ghost commented Mar 29, 2023

Thanks @yorkshire-pudding, committed. Thanks also to @quicksketch for opening this issue and filing the initial PR.

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

Successfully merging a pull request may close this issue.

2 participants