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

Hotfix #447 #449

Merged
merged 1 commit into from
Aug 22, 2019
Merged

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Aug 19, 2019

if we want not to loose compatibility with 10.2

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When creating the command from xml the exact same stuff is done as what has explicitly been coded now.

There is still something wrong if the code patch is not fixing the issue

appinfo/register_command.php Outdated Show resolved Hide resolved
appinfo/register_command.php Outdated Show resolved Hide resolved
@PVince81
Copy link
Contributor

not sure if the hotfix is really needed, considering that the original issue is with the apps/apps_external constellation for which there is a workaround: #447 (comment)

unless we want to send this patch to users and not release / merge it ?

@VicDeo
Copy link
Member Author

VicDeo commented Aug 19, 2019

@DeepDiver1975

When creating the command from xml the exact same stuff is done as what has explicitly been coded now.
There is still something wrong if the code patch is not fixing the issue

true.
but register_command is executed significantly later than info.xml is parsed. And when it is parsed wrong app.php is loaded. So the real fix is owncloud/core#36054

and this PR being a hotfix for the app is partially revert the changes from 360af33#diff-224bb875c5d3cfcd7621135069013242
This PR does exactly that. The same about authors - I just restored the original register_command.php, it had been not written by me.

@VicDeo
Copy link
Member Author

VicDeo commented Aug 19, 2019

@PVince81 I hadn't got any feedback so I decided to do both core fix for OC 10.3 and an app hotfix that will provide a seamless app upgrade experience for OC < 10.3 and potentially reduce number of support cases.
Surely it's up to management to decide whether hotfix is needed or not. :)

@PVince81
Copy link
Contributor

not wanting to merge this. People should update to 10.3 when it's out.

if needed, this PR can be sent as patch.

@pmaier1 @cdamken FYI

@PVince81 PVince81 closed this Aug 20, 2019
@pmaier1
Copy link
Contributor

pmaier1 commented Aug 20, 2019

Please make sure that there will be no issues in upgrading LDAP to 0.14.0, also on server versions below 10.3.

@PVince81 PVince81 reopened this Aug 20, 2019
@PVince81
Copy link
Contributor

@VicDeo please adjust based on comments

maybe also add a PHP comment somewhere as a note that the command is not in info.xml as a workaround, for the future.

we'll need to keep in mind to revert this again for the next LDAP app version 0.15.0 or later

@VicDeo VicDeo force-pushed the release-0.14.0-fix-multiappdir-update branch from 2dd22eb to 988185a Compare August 20, 2019 14:15
@VicDeo
Copy link
Member Author

VicDeo commented Aug 20, 2019

@PVince81 updated, added fixme

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@PVince81 PVince81 merged commit aece68d into release-0.14.0 Aug 22, 2019
@PVince81 PVince81 deleted the release-0.14.0-fix-multiappdir-update branch August 22, 2019 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants