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

Work on determination of "base_url" config item #2981

Merged
merged 8 commits into from Apr 1, 2014
Merged

Work on determination of "base_url" config item #2981

merged 8 commits into from Apr 1, 2014

Conversation

vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Mar 31, 2014

  • A little refactoring.
  • Remove the basename only at the end, to avoid edge cases.

Put the $_SERVER['HTTP_HOST'] fallback in a more logical place.
Remove the basename only at the end, to avoid edge cases.
Better performance by not using regex.
@vlakoff
Copy link
Contributor Author

vlakoff commented Mar 31, 2014

Also, shouldn't we remove the manual setting of this config item?
It seems unnecessary for long, are there some use cases for this?

@narfbg
Copy link
Contributor

narfbg commented Mar 31, 2014

It's not unnecessary, it's a must. This auto-detection is a fallback that no serious application should really use - it's not reliable.

What are the benefits from your refactoring?

@vlakoff
Copy link
Contributor Author

vlakoff commented Mar 31, 2014

The refactoring won't change anything in practice, but that's because the $_SERVER['HTTP_HOST'] fallback itself isn't really useful (at least in its current state).

edit: It looks like the autodetection would need some work to be usable in CLI.

@narfbg
Copy link
Contributor

narfbg commented Mar 31, 2014

I'm not really sure I understand you ... I don't see how it can be made more useful since it is based on an HTTP header.

Also, why is that test "unfit"? :)

@vlakoff
Copy link
Contributor Author

vlakoff commented Mar 31, 2014

I thought of it as useless, because making sense only on a local server and using a HTTP 1.0 client.

However I didn't think about CLI at the beginning.

Straight to the point, considering the results of autodetection in CLI, and my refactoring not improving that situation, I'd suggest leaving that part out, opening a new PR with only the changes of 2516f3b + 93836ff. Is this OK for you?

@narfbg
Copy link
Contributor

narfbg commented Mar 31, 2014

Ignore CLI, it's natural that you don't have a base URL under CLI. I'm still having a hard time understanding what you're trying to solve here though? Is this a performance improvement? Or fixing unexpected behavior?

@vlakoff
Copy link
Contributor Author

vlakoff commented Mar 31, 2014

It's in case the script name would be in the path too. Yes, this is veeery unlikely, but I think a robust code should not rely on unlikeliness.

@narfbg
Copy link
Contributor

narfbg commented Mar 31, 2014

Hehe, I'd sure would love to see who has their directory named 'index.php'. ;) But ok ... it makes sense. Just a few tips:

  • str_replace() is faster than strtr().
  • No spaces around concatenation.
  • Instead of preg_replace(), this should do:

    substr($_SERVER['SCRIPT_NAME'], 0, -strlen(basename($_SERVER['SCRIPT_NAME'])));

This one is great because we don't have to deal with the special cases:
* in Windows, dirname('/foo/index.php') gives "/foo", but dirname('/index.php') gives "\" instead of "/"
* dirname() doesn't include the trailing slash, with the expection of "/" (root)

props @narfbg
@vlakoff
Copy link
Contributor Author

vlakoff commented Mar 31, 2014

  • not for single-character replacements ;-)
  • ok
  • good catch, done in b208f65

Also, added a test (3802d70)

If you're OK with the changes, I'll make a streamlined PR, keeping this one for the sake of documentation.

@narfbg
Copy link
Contributor

narfbg commented Apr 1, 2014

No, this one is fine. :)

narfbg added a commit that referenced this pull request Apr 1, 2014
Work on determination of "base_url" config item
@narfbg narfbg merged commit b749fab into bcit-ci:develop Apr 1, 2014
@aanbar
Copy link
Contributor

aanbar commented Apr 2, 2014

lines 89 & 90 are not needed anymore in Config_test.php

@narfbg
Copy link
Contributor

narfbg commented Apr 2, 2014

They are.

@aanbar
Copy link
Contributor

aanbar commented Apr 2, 2014

These are already defined in line 70-72 am I missing something?

@narfbg
Copy link
Contributor

narfbg commented Apr 2, 2014

Yes, you are. :)

@aanbar
Copy link
Contributor

aanbar commented Apr 2, 2014

Just noticed it, You're modifying the _SERVER

@vlakoff vlakoff deleted the base_url branch April 11, 2014 11:24
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.

None yet

3 participants