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

Improve _replace_variable_by_path #550

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bsuttor
Copy link
Member

@bsuttor bsuttor commented Apr 22, 2020

Return an url and not a path

At this moment we use "/".join(obj.getPhysicalPath()) to replace
${navigation_root_url} and ${portal_url} in Link redirection indexer but with
production site (with reverse proxy), it seems use acquisition to get correct link.

Example for same portal in production and in development:

production
https://mysite.com/my-folder/my-link
with remoteUrl = ${navigation_root_url}/my-other-folder

devl
http://localhost:8080/plone/my-folder/my-link
with same remoteUrl

generated link in dev is good (http://localhost:8080/plone/my-other-folder/)
but in production, it's not correct, indeed it's
https://mysite.com/plone/my-other-folder/
and it should be
https://mysite.com/my-other-folder/

'plone' path should not be in the link.
It maybe works with acquisition, but it's not correct .
So I simply replace '/'.join(obj.getPhysicalPath()) by
obj.absolute_url()

Is there good reason to use getPhysicalPath instead of absolute_url?

Here is how to reproduce with docker-compose.yml file and a reverse proxy :

version: '3'
services:
  zeo:
    image: plone:5.2.1-alpine
    command: zeo
  instance:
    image: plone:5.2.1-alpine
    ports:
      - 8080:8080
    environment:
      - ZEO_ADDRESS=zeo:8080
    links:
      - zeo
    depends_on:
      - reverseproxy
    labels:
      - 'traefik.enable=true'
      - 'traefik.http.routers.instance.rule=Host(`portal.localhost`)'
      - 'traefik.http.routers.instance.entrypoints=web'
      - 'traefik.http.services.instance.loadbalancer.server.port=8080'
      - "traefik.http.routers.instance.middlewares=add-plone"
      - "traefik.http.middlewares.add-plone.addprefix.prefix=/VirtualHostBase/http/portal.localhost/plone/VirtualHostRoot"

  reverseproxy:
    image: traefik
    command:
      - '--api.insecure=true'
      - '--providers.docker=true'
      - '--entryPoints.web.address=:80'
    ports:
      - '80:80' # The HTTP port
      - '8000:8080' # The Web UI (enabled by --api)
    volumes:
      - /var/run/docker.sock:/var/run/docker.sock # So that Traefik can listen to the Docker events

then you have

We’re sorry, but there seems to be an error…

If you are certain you have the correct web address but are encountering an error, please contact the site administration.

Return an url and not a path
@mister-roboto
Copy link

@bsuttor thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@bsuttor
Copy link
Member Author

bsuttor commented Apr 22, 2020

@jenkins-plone-org please run jobs

@bsuttor
Copy link
Member Author

bsuttor commented Apr 23, 2020

@jenkins-plone-org please run jobs

@NicolasGoeddel
Copy link

Can this be fixed soon?

@bsuttor
Copy link
Member Author

bsuttor commented Oct 13, 2020

I'm not sure this is the good way to fix this. It's seems setting "plone" as site id is not a good choice. Indeed, we had some other issues

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.

3 participants