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

related feed URL incorrect #9

Closed
cweiske opened this Issue Jan 3, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@cweiske

cweiske commented Jan 3, 2017

https://cweiske.de/tagebuch/feed/index.htm has a related feed URL, but micrometa does not return the correct one.

micrometa: http://cweiske.de/feed/
correct: http://cweiske.de/tagebuch/feed/

Maybe some issue with base href parsing.

URL: https://cweiske.de/tagebuch/feed/index.htm
Base href: <base href="../"/>
Feed URL: <link rel="alternate" type="application/atom+xml" title="Christians Tagebuch" href="feed/" />

@cweiske

This comment has been minimized.

cweiske commented Jan 3, 2017

The URLs of all h-entries are incorrect, too.

@jkphl jkphl self-assigned this Jan 3, 2017

@jkphl

This comment has been minimized.

Owner

jkphl commented Jan 3, 2017

@cweiske Thanks for reporting, Christian. I will try to look into this asap!

@jkphl jkphl added the bug label Jan 3, 2017

jkphl added a commit that referenced this issue Jan 3, 2017

@jkphl

This comment has been minimized.

Owner

jkphl commented Jan 3, 2017

@cweiske I could improve relative base URL handling in micrometa a little, but the real problem seems to lie in php-mf2 rather than in micrometa. According to the inline comments there you must be high ;) and the resolveUrl() function doesn't return the result you expect. Just do a simple call to to resolveUrl('https://cweiske.de/tagebuch/feed/index.htm ', '../') and see yourself...

Would you mind openig an issue with your case over there?

@jkphl jkphl closed this Jan 3, 2017

@cweiske

This comment has been minimized.

cweiske commented Jan 4, 2017

Strangely, http://pin13.net/mf2/?url=https%3A%2F%2Fcweiske.de%2Ftagebuch%2Ffeed%2Findex.htm does list the correct URLs - and it uses php-mf2, too.

@jkphl

This comment has been minimized.

Owner

jkphl commented Jan 4, 2017

@cweiske Hmm. Then there's one explanation: The resolution of the alternate URL in pin13 doesn't rely on the (false) determination of the base URL by php-mf2. I will again look into that and see if I could do the same for micrometa. Thanks again.

@jkphl jkphl reopened this Jan 4, 2017

@cweiske

This comment has been minimized.

cweiske commented Jan 4, 2017

No, php-mf2 should be fixed.

@jkphl

This comment has been minimized.

Owner

jkphl commented Jan 4, 2017

@cweiske In any case. But still, just in case that php-mf2 isn't responsible for alternate URL resolution anyway, micrometa needs to be fixed as well (just as Aaron probably did with pin13).

@cweiske

This comment has been minimized.

cweiske commented Jan 4, 2017

resolveUrl() does seem to be correct:

$fullbase = mf2\resolveUrl('https://cweiske.de/tagebuch/feed/index.htm', '../');
$feed = mf2\resolveUrl($fullbase, 'feed/');
var_dump($fullbase, $feed);

gives:

string(28) "https://cweiske.de/tagebuch/"
string(33) "https://cweiske.de/tagebuch/feed/"

This is correct.

Or did you want to say that php-mf2 ignores the base url tag?

@cweiske

This comment has been minimized.

cweiske commented Jan 4, 2017

No, php-mf2 is correct. When using its Mf2\parse() method, all URLs come back correctly.

@jkphl

This comment has been minimized.

Owner

jkphl commented Jan 4, 2017

@cweiske That is super strange! I did the very same test yesterday, and calling resolveUrl definitely screwed the URL. I will look into that again later today. Thanks for examining!

@jkphl jkphl closed this in c7934b2 Jan 4, 2017

@jkphl

This comment has been minimized.

Owner

jkphl commented Jan 4, 2017

@cweiske Hmm ... must have overseen this yesterday, but my changes yesterday seem to have fixed this issue. Pushing out a bugfix release now ... Thanks again!

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