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

Open graph fully absolute urls support #947

Closed

Conversation

guiohm
Copy link

@guiohm guiohm commented Dec 6, 2014

I don't quite understand why the optional subdirectory had to be added on both config.urland config.root.
I believe this is more relevant now.

…e urls for OG image tags. Also fix issues with site url when installed in subdirectory
@guiohm guiohm mentioned this pull request Dec 6, 2014
@leesei
Copy link
Member

leesei commented Dec 9, 2014

Are #942, #943 related?

The link for the sites title should be config.url + config.root (better use url.resolve() for url concatenation), can you verify that?

@guiohm
Copy link
Author

guiohm commented Dec 9, 2014

This fixes #942 but not #943 (I might look into that later).

I don't see where is the site title link you're talking about?

My last commit adds url.resolve in 2 places, it should be done elsewhere but I think we should count on sanitized settings (and improve it) made when loading config: https://github.com/hexojs/hexo/blob/master/lib/loaders/config.js#L110-116

What do you think?

@guiohm
Copy link
Author

guiohm commented Dec 9, 2014

In the meantime, I'm searching how to fix #943. I think that no code exist to handle rewriting all urls in a post, right?
The closest one I found is the filter for external links detection here: https://github.com/hexojs/hexo/blob/master/lib/plugins/filter/external_link.js

So do we want to add another filter for that? or adding it to the existing one?
We need to rewrite a tags and img tags as well by the way.

[If I do that, I'll open another pull request]

@leesei
Copy link
Member

leesei commented Dec 9, 2014

Thanks for your effort.
I agree that it's better to normalize the site's url in one place (config) rather than letting each plugin/theme to recompute it.

What I meant by the site url is:

I'm not sure if the whole "child directory" feature was intended to implement the last use case, which is kind of link http://nodejs.org/ and http://nodejs.org/blog.


If links are not rewritten, that means this PR is a bugfix in Hexo internal and possibly relates to theme developers.
While the blog writer still need some tag for internal links (#458), otherwise the config.root and permalinks would be all over the .md files, rendering them un-portable. It would be a vast effort to change config.root or permalink format (whether they should be changed is another issue).

I brought the internal links issue up since these two issues both relates to the interpretation and use case of config.url and config.root.

@guiohm
Copy link
Author

guiohm commented Dec 11, 2014

So I added the last piece of normalization of config.url and config.root at config load time.
I checked the site home/title url issue and no issue for me with this PR.

About the internal links in .md files, beside a helper that would obviously help. I believe we should really consider adding a filter to detect links to images or any other file already added manually or via other tag helpers.
For instance I'm adding images manually in my posts (located in source/images), so I should write this:
<img src="/images/cords-237x300.jpg" alt="attachment cords" class="image-left" />
But I need instead to put:
<img src="/blog/images/cords-237x300.jpg" alt="attachment cords" class="image-left" />

I believe that config.url and config.root are now setup to suit any use-case. So development should continue based on that.
I might very well write a filter to address my issue in the next few days, at least temporarily.

@guiohm guiohm closed this Dec 11, 2014
@guiohm
Copy link
Author

guiohm commented Dec 11, 2014

Wooops, I mistakenly closed the PR....

@guiohm guiohm reopened this Dec 11, 2014
@guiohm
Copy link
Author

guiohm commented Dec 11, 2014

Hummm... The Travis build 504 (https://travis-ci.org/hexojs/hexo/builds/43772973) is locked currently. I suppose some admin needs to re-trigger the build?

@guiohm
Copy link
Author

guiohm commented Dec 11, 2014

Well, the build timed out... I don't ses any restart button. Anyone?

@leesei
Copy link
Member

leesei commented Dec 12, 2014

As a site note (as I found in #943), if you enable config.relative_link,

<img src="images/cords-237x300.jpg" alt="attachment cords" class="image-left" />

note the absence of '/' prefix

the src url will take config.root into consideration. Please try it out.

@guiohm
Copy link
Author

guiohm commented Dec 12, 2014

config.relative_link and not using '/' won't work because the post is displayed on multiple pages (post page, index, tags, archives, categories) and the url would need to be rewritten differently on each page.

Same issue If I activate post_asset_folder and if I put the image in the asset folder of the post.

The only way for me to make it work on all pages is to use non-relative urls in src tags and adding the config.root manually.

@Xuanwo
Copy link
Contributor

Xuanwo commented Dec 4, 2015

The build was gone for so long that I can't rebuild it, make a new pr or commit may restart it.

@stale stale bot added the wontfix This will not be worked on label Sep 27, 2017
@stale
Copy link

stale bot commented Sep 27, 2017

This issue has been automatically marked as stale because lack of recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 26, 2017
@stale stale bot closed this Dec 3, 2017
@github-actions
Copy link

How to test

git clone -b open_graph_fully_absolute_urls_support https://github.com/guiohm/hexo.git
cd hexo
npm install
npm test

@stevenjoezhang stevenjoezhang removed stale wontfix This will not be worked on labels Mar 27, 2022
@hexojs hexojs deleted a comment from stale bot Mar 28, 2022
@stevenjoezhang
Copy link
Member

Superseded by #4616
root option is now optional

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.

4 participants