Fix to use root to export Permalink #3604

sKawashima · 2019-06-29T10:23:38Z

What does it do?

Fix to use root to export permalink.
It fix #3601 .

[Page,Category,Tag].virtual('permalink').get(function() {
// return `${ctx.config.url}/${this.path}`; 
// to
  return `${ctx.config.url}${ctx.config.root}${this.path}`;
});

How to test

git clone -b BRANCH https://github.com/USER/hexo.git
cd hexo
npm install
npm test

coveralls · 2019-06-29T10:28:53Z

Coverage remained the same at 97.148% when pulling a423068 on sKawashima:20190629fixPermalink_addRoot into 7fba3c1 on hexojs:master.

coveralls · 2019-06-29T10:28:53Z

Coverage remained the same at 97.148% when pulling a423068 on sKawashima:20190629fixPermalink_addRoot into 7fba3c1 on hexojs:master.

sKawashima · 2019-07-01T03:31:38Z

Codeclimate is not passed, but it was so before I touched it.

Please check.

yoshinorin · 2019-07-01T09:57:05Z

Sorry, We have many repositories and need to maintain them.
Would you please give us time? Thanks 😃

curbengh · 2019-07-10T04:04:08Z

@#2610 (comment)

In this case, why is root parameter for? Just for curiosity...

It's used in several tag plugins like include code to link to a local path via <a href="/root_value/filename/"> instead of <a href="http://yourweb.com/root_value/filename/">,

hexo/lib/plugins/tag/include_code.js

Line 59 in da14a16

    
           const caption = `<span>${title}</span><a href="${ctx.config.root}${codeDir}${path}">view raw</a>`;

Post model replace the config.root with '/',

hexo/lib/models/post.js

Line 57 in f172774

return config.url + partial_url.replace(config.root, '/');

The above works because the docs requires root value in both url and root.
https://github.com/hexojs/hexo-starter/blob/e5e9b59a5b66d4468467af4f1e931988aaa7047b/_config.yml#L14-L16

Alternatively, we can specify just one root value:

url: http://example.com
root: /blog/

The issue with this approach is that <%= config.url %> would be invalid, the proper one should be <%= config.url + config.root %>. This can be fixed by additional processing to append config.root to config.url, so config.url variable includes config.root. However, this also means that config.url no longer corresponds to the value in _config.yml.

I'm not so worry about the breaking change of the above approach, since v4 is on the horizon.

@yoshinorin
The docs mention The URL of current page without root URL. We usually use url_for(page.path) in theme.. Shouldn't page.path includes config.root? If that's the case, then it should fix the issue.

curbengh · 2019-07-10T04:21:03Z

Currently the config requires root value in both url and root,
https://github.com/hexojs/hexo-starter/blob/e5e9b59a5b66d4468467af4f1e931988aaa7047b/_config.yml#L14-L16

For example,

url: http://yoursite.com/child
root: /child/

then

return `${ctx.config.url}${ctx.config.root}${this.path}`;

will return http://yoursite.com/child/child/path.

yoshinorin · 2019-07-10T08:22:10Z

@weyusi
Thank you for your support :)

url & root will be fixed following lines.

hexo/lib/hexo/load_config.js

Lines 32 to 33 in f172774

    
           config.root = config.root.replace(/\/*$/, '/'); 
        
           config.url = config.url.replace(/\/+$/, '');

I think almost certainly this PR is okay. I'm going to approve and merge this PR. But, I'm not yet completed check current hexo source code. So, I haven't got confirmation of that yet.

curbengh · 2019-07-10T09:44:43Z

@yoshinorin
I just tested the regex,

config.root.replace(/\/*$/, '/');

this make sure the value ends with a slash, '/child' -> '/child/'

config.url.replace(/\/+$/, '');

this make sure the url does not ends with a slash,

'http://example.com/' -> 'http://example.com'
'http://example.com/child/ -> 'http://example.com/child'.

Anyhow, 'child' value is still there. I just tested this PR and it definitely results in http://yoursite.com/child/child/path.

For this PR to work, '/child' needs to be removed from config.url (either ask user to update their _config.yml or programmatically via load_config.js).

yoshinorin · 2019-07-10T13:39:00Z

@weyusi
Just to confirm...

NOTE: I do not confirm it on the current master source code. Just confirmed on a console.

Current hexo config & result

The URL of current page without root URL. We usually use url_for(page.path) in theme...

My understanding its means...

Pattern 1: OK

Settings

url: http://yoursite.com
root: /

Result

const urlOne = "http://example.com".replace(/\/+$/, '');
const rootOne = "/".replace(/\/*$/, '/');

console.log('urlOne: ' + urlOne);
console.log('rootOne: ' + rootOne);
console.log('pattern1: ' + urlOne + rootOne + "\n");

// Result
urlOne: http://example.com
rootOne: /
pattern1: http://example.com/

Pattern 2: OK

Settings

url: http://yoursite.com
root: /child

Result

const urlTwo = "http://example.com".replace(/\/+$/, '');
const rootTwo = "/child".replace(/\/*$/, '/');

console.log('urlTwo: ' + urlTwo);
console.log('rootTwo: ' + rootTwo);
console.log('pattern2: ' + urlTwo + rootTwo + "\n");

// Result
urlTwo: http://example.com
rootTwo: /child/
pattern2: http://example.com/child/

Pattern 3: OK

Settings

url: http://yoursite.com/
root: /child

Result

const urlThree = "http://yoursite.com/".replace(/\/+$/, '');
const rootThree = "/child".replace(/\/*$/, '/');

console.log('urlThree: ' + urlThree);
console.log('rootThree: ' + rootThree);
console.log('pattern3: ' + urlThree + rootThree + "\n");

// Result
urlThree: http://yoursite.com
rootThree: /child/
pattern3: http://yoursite.com/child/

Pattern 4: NG

Settings

url: http://yoursite.com/
root: child

Result

const urlFour = "http://yoursite.com/".replace(/\/+$/, '');
const rootFour = "child".replace(/\/*$/, '/');

console.log('urlFour: ' + urlFour);
console.log('rootFour: ' + rootThree);
console.log('pattern4: ' + urlFour + rootFour + "\n");

// Result
urlFour: http://yoursite.com
rootFour: /child/
pattern4: http://yoursite.comchild/ <- it's wrong. But not relate this PR

My understanding is above. Is it okay??

About this PR

This PR is not change load_config.js.
Maybe this PR source code will not affect url + root path.

So, I think maybe this PR is not breaking change & merge safely.

How do you think? :)

curbengh · 2019-07-11T05:53:52Z

url: http://yoursite.com/child
root: /child/

is the current approach. While I don't necessarily agree with that approach, unless the current approach is updated, this PR is a breaking change.

My suggestion is to switch to the following approach (a breaking change),

url: http://yoursite.com
root: /child

Then change,

hexo/lib/hexo/load_config.js

Line 33 in f172774

config.url = config.url.replace(/\/+$/, '');

to

    config.url = config.url.replace(/\/+$/, '').concat(config.root);

This will also fix #3601, without this PR.

curbengh · 2019-07-12T04:01:30Z

Maybe @sKawashima @yoshinorin haven't realize,
#3601 can also be easily fixed by,

url: http://yoursite.com/child
root: /child/

which is (I iterate again) the current approach. I understand @sKawashima might not like that approach, me neither. Most users either has "/" as root: value or grown accustomed to having "/child" in both url: and root:.

The rationale of using url: http://yoursite.com/child instead of url: http://yoursite.com/ (when root: is other than "/") is so that users can use <%= config.url %> to add a link to their home page, instead of using <%= config.url + config.root %>. I'm iterating my previous comment here.

If user wants to use,

url: http://yoursite.com/
root: /child/

without using <%= config.url + config.root %>, then hexo needs

config.url = config.url.replace(/\/+$/, '').concat(config.root);

in hexo/lib/hexo/load_config.js.

So, the real question is, should

url: http://yoursite.com/child
root: /child/

be deprecated, in favor of

url: http://yoursite.com/
root: /child/

?

sKawashima · 2019-07-12T06:47:53Z

@weyusi
So why does $ {ctx.config.url}, $ {ctx.config.root} and $ {this.path} exist in this source code?

I have tried it once, but I sent this PR because I had a few problems.
There are the behavior of link generation for share, the file arrangement at the time of generate, and the behavior of link at the time of rss generation in hexo-generator-feed.

I think that it is not consistent about url extraction of permalink and $ hexo generate.

yoshinorin · 2019-07-13T03:56:04Z

@weyusi @sKawashima
It's very sorry. I perfectly misunderstood.
I completely thought _config.yml setting is already as following.

url: http://yoursite.com/
root: /child/

Sorry for the confusion...
I will read related issue & current source code again.

curbengh · 2019-07-15T08:56:33Z

So why does $ {ctx.config.url}, $ {ctx.config.root} and $ {this.path} exist in this source code?

Good question,
when you have the following config,

url: http://yoursite.com/child
root: /child/

and your page is located at source/examplepage/index.md,

Variable	Value
ctx.config.url	`http://yoursite.com/child`
ctx.config.root	`/child/`
page.path	`examplepage/index.html`
page.permalink	`ctx.config.url` + `/` + `page.path` = `http://yoursite.com/child/examplepage/index.html`

As for the purpose of ctx.config.root, as mentioned in my previous comment, it serves as another way to link to a same-origin page or assets.

Instead of using permalink which results in <a href="http://yoursite.com/child/examplepage/index.html">, hexo also uses ctx.config.root + page.path which results in <a href="/child/examplepage/index.html">. I can't confirm the exact rationale, but my guess is this approach allows distinguish of local and third-party links.

If you can pinpoint the exact issue (e.g. expected urls vs actual urls), it can help us to troubleshoot.

sKawashima · 2019-11-20T09:19:45Z

I'm sorry for the late reply.
Thank you.

sKawashima added 2 commits June 29, 2019 19:12

feat:create pull request

19bec9a

fix virtual(permalink).get()

a423068

yoshinorin added the bug Something isn't working label Jul 2, 2019

yoshinorin self-assigned this Jul 2, 2019

curbengh added the Breaking Change label Jul 10, 2019

curbengh mentioned this pull request Jul 20, 2019

The project root directory is missing hexojs/hexo-generator-feed#74

Closed

curbengh mentioned this pull request Aug 10, 2019

Fixed data URL bug with share button hexojs/hexo-theme-landscape#113

Closed

SukkaW mentioned this pull request Sep 4, 2019

feat(full_url_for): utilize hexo-util module #3701

Merged

2 tasks

sKawashima closed this Nov 20, 2019

stevenjoezhang mentioned this pull request Nov 20, 2019

permalink for pages broken for when config.root #2105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to use root to export Permalink #3604

Fix to use root to export Permalink #3604

sKawashima commented Jun 29, 2019

coveralls commented Jun 29, 2019

coveralls commented Jun 29, 2019

sKawashima commented Jul 1, 2019

yoshinorin commented Jul 1, 2019

curbengh commented Jul 10, 2019 •

edited

Loading

curbengh commented Jul 10, 2019

yoshinorin commented Jul 10, 2019 •

edited

Loading

curbengh commented Jul 10, 2019 •

edited

Loading

yoshinorin commented Jul 10, 2019 •

edited

Loading

curbengh commented Jul 11, 2019

curbengh commented Jul 12, 2019

sKawashima commented Jul 12, 2019 •

edited

Loading

yoshinorin commented Jul 13, 2019 •

edited

Loading

curbengh commented Jul 15, 2019 •

edited

Loading

sKawashima commented Nov 20, 2019

Fix to use root to export Permalink #3604

Fix to use root to export Permalink #3604

Conversation

sKawashima commented Jun 29, 2019

What does it do?

How to test

coveralls commented Jun 29, 2019

coveralls commented Jun 29, 2019

sKawashima commented Jul 1, 2019

yoshinorin commented Jul 1, 2019

curbengh commented Jul 10, 2019 • edited Loading

curbengh commented Jul 10, 2019

yoshinorin commented Jul 10, 2019 • edited Loading

curbengh commented Jul 10, 2019 • edited Loading

yoshinorin commented Jul 10, 2019 • edited Loading

Current hexo config & result

Pattern 1: OK

Pattern 2: OK

Pattern 3: OK

Pattern 4: NG

About this PR

curbengh commented Jul 11, 2019

curbengh commented Jul 12, 2019

sKawashima commented Jul 12, 2019 • edited Loading

yoshinorin commented Jul 13, 2019 • edited Loading

curbengh commented Jul 15, 2019 • edited Loading

sKawashima commented Nov 20, 2019

curbengh commented Jul 10, 2019 •

edited

Loading

yoshinorin commented Jul 10, 2019 •

edited

Loading

curbengh commented Jul 10, 2019 •

edited

Loading

yoshinorin commented Jul 10, 2019 •

edited

Loading

sKawashima commented Jul 12, 2019 •

edited

Loading

yoshinorin commented Jul 13, 2019 •

edited

Loading

curbengh commented Jul 15, 2019 •

edited

Loading