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

fix url::makeAbsolute() and url::short() #213

Open
wants to merge 8 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@sebsel
Contributor

sebsel commented Jan 18, 2017

Instead of filing issues I now try to fix issues :)

I had a bug that came down to relying on url::makeAbsolute() to fix a relative link. Turned out url::makeAbsolute() doesn't actually solve relative links, it just intelligently adds $path to $home.

So I made a few tests for urls it should solve in my opinion, and then I rewrote the function. It now passes all those tests, and also the tests on $home as /, because that seems to be the default.

I also noticed that url::short() would turn the url http://no-www.org into no-org, because of a str_replace() on www., which isn't correct also, so I fixed that too.

@sebsel

This comment has been minimized.

Contributor

sebsel commented Jan 25, 2017

I found a new case which it didn't solve (https://webmention.rocks/test/22), so I added a test for that too. And then I had to completely redo the function again. I think this is a way better approach: it now filters and alters the fragments as an array, and calls url::build() with those. It's more readable too.

Also had to fix url::fragments(), which would return null when empty, instead of []. I saw the similar url::params() already returned [] in that case.

@sebsel

This comment has been minimized.

Contributor

sebsel commented Jan 25, 2017

Sad to admit, but this does potentially break some things. I had the following code on my site:

  <?php echo css('assets/css/main.css') ?>

Which worked relative to my root. Now it breaks, because it's relative to the page. Luckily it is as easy as adding a / at the beginning. It's more a problem in my code, because it's more logical to interpret this as a relative path here.

Unfortunately, this exact code is also present in Kirby's Starterkit. Don't know how you guys want to handle that.

Edit: also found that (of course) go() uses this function too.

@sebsel

This comment has been minimized.

Contributor

sebsel commented Feb 6, 2017

Would it help to rename this function to url::solveRelative(), so you can keep url::makeAbsolute() for the use it has now?

The toolkit just really needs a function like this.

Also: relative urls like ../ and ./ might need to be addressed too. I can try and add that as well, but I would like some response to this PR first :)

@bastianallgeier

This comment has been minimized.

Contributor

bastianallgeier commented Feb 8, 2017

Wow, you put quite a lot of work into this. I think in order to keep it backward compatible I'd prefer to make it a new function.

@sebsel

This comment has been minimized.

Contributor

sebsel commented Feb 8, 2017

Okay, I renamed the function. While I was doing so, it made sense to switch the order of the params, and make them both required. This is now a function to solve relativeness from a $base to another url. (I kept the second param as $path though, because $url would be confusing.)

Also renamed the tests, got rid of the tests with only a $path, and changed the wording on the doc on url::makeAbsolute(). :)

Seems like the checks fail, updating my repo now..

@sebsel

This comment has been minimized.

Contributor

sebsel commented Feb 8, 2017

Checks fail on the MessageFormatter extension for me :(

@sebsel sebsel referenced this pull request Jun 29, 2017

Closed

pings.json empty #9

sebsel added a commit to sebsel/seblog-kirby-webmentions that referenced this pull request Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment