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

Fix change url function #608

Open
wants to merge 5 commits into
base: stretch-unstable
from

Conversation

Projects
None yet
3 participants
@Josue-T
Copy link
Contributor

Josue-T commented Dec 30, 2018

The problem

  • In the change-url function we use a old function check_url.
  • The change-url function call the change-url script with variable with empty value ($YNH_APP_OLD_PATH, $YNH_APP_NEW_PATH ) when the path is the root path. By this we need write a code like that : https://github.com/YunoHost-Apps/wordpress_ynh/blob/master/scripts/change_url#L41-L44 . In the same time we have in the core a function which do that : _normalize_domain_path.

Solution

  • Replace the check_url by _get_conflicting_apps.
  • Give the argument about the url as same as in the install.

PR Status

Tested. Work on my side.

Note that this PR should also fix #376

How to test

Test with an app which has a change-url script.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@Josue-T Josue-T requested review from Psycojoker , zamentur and alexAubin Dec 30, 2018

@Josue-T Josue-T referenced this pull request Dec 30, 2018

Open

Group permission #585

5 of 14 tasks complete

Josue-T added some commits Dec 30, 2018

@alexAubin
Copy link
Member

alexAubin left a comment

Quick review (not tested) : LGTM

@Psycojoker
Copy link
Member

Psycojoker left a comment

It seems that there are subtles modifications about trailing "/" in path, I'm not really confident that this won't break yunohost in some unexpected ways.

@@ -1303,8 +1309,7 @@ def app_checkurl(auth, url, app=None):
path = url[url.index('/'):]
installed = False

if path[-1:] != '/':
path = path + '/'
domain, path = _normalize_domain_path(domain, path)

This comment has been minimized.

@Psycojoker

Psycojoker Jan 7, 2019

Member

_normalize_domain_path doesn't had a trailing "/", this code replacement is not equivalent.

This comment has been minimized.

@Josue-T

Josue-T Jan 7, 2019

Contributor

This is to keep the same result than the register_url. Actually the register_url function use this function. I think we need to be coherent.

@alexAubin
Copy link
Member

alexAubin left a comment

Tested and works okay

(I pushed a small fix because asking for 'Main domain' (interactively, if domain is not specified) is pretty confusing)

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