-
Notifications
You must be signed in to change notification settings - Fork 32
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
Transfer deprecated ajax files to appframework #39
Conversation
6d11261
to
7c1c9c6
Compare
In Nextcloud 9 the camel-casing of the files and directories isn't supported, that's why the stable9 builds are failing: https://travis-ci.org/nextcloud/jsxc.nextcloud/jobs/256023865. This only is supported since PSR4 which was introduced in oC 9.1 (NC 10) see: https://mailman.owncloud.org/pipermail/devel/2016-May/002455.html and owncloud/core#24386 and an example for an app owncloud/announcementcenter#73 |
So can we drop support for NC9? #40 |
- inject raw request body - use SignatureProtectedApiController to require signature in header
- rename externalApi to api - add version
@MarcelWaldvogel The new API will be @LEDfan please have a look at the code, because you have more experience. I bumped phpunit to 5.7 to get the new cool |
foreach($userGroup->getUsers() as $user) { | ||
$uid = $user->getUID(); | ||
|
||
if(!$roster[$uid]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably better to use http://php.net/manual/en/function.array-key-exists.php, this will give a warning https://travis-ci.org/nextcloud/jsxc.nextcloud/jobs/256943398#L377
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈 this was probably my js background
lib/TimeLimitedToken.php
Outdated
namespace OCA\OJSXC; | ||
|
||
class TimeLimitedToken { | ||
public static function generateUser($node, $domain, $secret, $ttl = 60*60, $time) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the $time parameter was added later, the optional parameter should be at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it for testing.
Wow good job! I only have two remarks (see above). |
That's a great idea. Maybe also https://codecov.io? See https://github.com/codecov/example-php. |
Thanks for reviewing. |
I thought it's maybe a good idea to transfer all deprecated ajax files to the more common appframework structure. This way we maybe satisfy the app code checker someday 🤣.
@LEDfan I changed the casing of your controller and the folder, because I like camel-case more, but in some issue you said something about backwards-compatibility. Can you explain this more?
Tasks: