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

link_prefix does not work as expected #516

Closed
inytar opened this issue Feb 14, 2017 · 4 comments
Closed

link_prefix does not work as expected #516

inytar opened this issue Feb 14, 2017 · 4 comments

Comments

@inytar
Copy link

inytar commented Feb 14, 2017

As mentioned on discord link_prefix does not do work as expected. Using the code in this gist: https://gist.github.com/inytar/5cb0b7b4d192e22f83180ff99a2cc8be I expect to see:

{
"external_link_def": "http://external.com/external",
"external_link_expl": "http://external.com/external",
"internal_link": "http://127.0.0.1:5000/"
}

but get:

{
"external_link_def": "http://127.0.0.1:5000/external",
"external_link_expl": "http://127.0.0.1:5000/external",
"internal_link": "http://127.0.0.1:5000/"
}

I'm using morepath 0.17 on Python 3.5.

Some more experimenting shows that only the root App's link_prefix is used.

(As a discussion point: what should you expect when mounting an app on an other app? Use the link_prefix of the child or the parent app?)

@faassen
Copy link
Member

faassen commented Feb 16, 2017

@href, opinions please? I had in my head that this ought to work for link generation but there is something to say for controlling this globally.

Do we need two prefix mechanisms, one global and one per app, with second second overriding the first? I need to consider the implementation as I don't yet understand why it works globally.

@href
Copy link
Member

href commented Feb 16, 2017

The earliest I can look at this is by the end of next week. The conference I'm co-organising is tomorrow ;)

@href
Copy link
Member

href commented Feb 23, 2017

In my opinion this is a bug. The link_prefix code never took mounted applications into account. The referenced commit/pull request fixes this issue. Please let me know what you think @faassen.

Also thank you @inytar for reporting this issue. I took the liberty of including your code as a test-case.

@henri-hulski
Copy link
Member

Seems to be fixed.

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

No branches or pull requests

4 participants