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.

Sign up for GitHub

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

Jump to bottom

Remove unused methods of OC_Json #8791

Merged
merged 1 commit into from
Mar 22, 2018
Merged

Remove unused methods of OC_Json #8791

merged 1 commit into from
Mar 22, 2018

Conversation

MorrisJobke
Copy link
Member

Ref #8375 and #7827

@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone Mar 12, 2018
ChristophWurst
ChristophWurst reviewed Mar 12, 2018
View reviewed changes
lib/private/legacy/api.php Outdated
@@ -485,7 +485,10 @@ public static function renderResult($format, $meta, $data) {
),
);
if ($format == 'json') {
return OC_JSON::encode($response);
if (is_array($response)) {
array_walk_recursive($response, array('OC_JSON', 'to_string'));
Copy link
Member

Choose a reason for hiding this comment

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

What kind of dark hackery is this

server/lib/private/legacy/json.php

Line 173 in fac0a11

$value = (string)$value;
? 🙈

Copy link
Member

Choose a reason for hiding this comment

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

Do not ask question you don't want answered

Copy link
Member

Choose a reason for hiding this comment

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

🤐

Copy link
Member Author

Choose a reason for hiding this comment

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

It's basically an object which use cast to a string (calling then the __toString() method explicitly). Otherwise the json_encode would fail with an error, that it only can serialize strings, integer, bool and date (the types it knows how to serialize).

lib/private/legacy/eventsource.php Outdated
@@ -89,6 +89,13 @@ protected function init() {
flush();
}

private function encode($data) {
if (is_array($data)) {
array_walk_recursive($data, array('OC_JSON', 'to_string'));
Copy link
Member

Choose a reason for hiding this comment

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

That's not even a public method

server/lib/private/legacy/json.php

Line 171 in fac0a11

protected static function to_string(&$value) {
🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

:/ Need to work on this yes :/

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 12, 2018
@codecov
Copy link

codecov bot commented Mar 13, 2018 •
edited
Loading

Codecov Report

Merging #8791 into master will increase coverage by 0.04%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #8791      +/-   ##
============================================
+ Coverage     51.89%   51.93%   +0.04%     
- Complexity    25278    25279       +1     
============================================
  Files          1604     1607       +3     
  Lines         94798    94872      +74     
  Branches       1377     1377              
============================================
+ Hits          49193    49275      +82     
+ Misses        45605    45597       -8
Impacted Files Coverage Δ Complexity Δ
lib/public/JSON.php 0% <ø> (ø) 6 <0> (-1) ⬇️
lib/private/legacy/eventsource.php 0% <0%> (ø) 13 <0> (ø) ⬇️
lib/private/legacy/json.php 12% <0%> (+1.28%) 19 <0> (-4) ⬇️
lib/public/DB.php 25% <0%> (-75%) 4% <0%> (+3%)
...aring/lib/Controller/MountPublicLinkController.php 21.52% <0%> (-20.37%) 24% <0%> (+10%)
lib/public/Share.php 35.29% <0%> (-4.71%) 7% <0%> (+1%)
settings/Controller/GroupsController.php 64.61% <0%> (-3.72%) 9% <0%> (ø)
lib/public/Util.php 56.88% <0%> (-3.32%) 48% <0%> (+3%)
apps/dav/lib/CalDAV/Activity/Provider/Base.php 76.78% <0%> (-2.58%) 16% <0%> (-3%)
apps/updatenotification/lib/UpdateChecker.php 71.42% <0%> (-1.3%) 8% <0%> (ø)
... and 45 more

@MorrisJobke MorrisJobke mentioned this pull request Mar 13, 2018
Merged
@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 13, 2018
@MorrisJobke
Copy link
Member Author

Ready for review again. I dropped the public interfaces that aren't used outside of lib/private and migrated the remaining ones to the private methods.

rullzer
rullzer approved these changes Mar 13, 2018
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

🔥

@MorrisJobke
Remove unused methods of OC_Json
78211a5 
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Copy link
Member Author

@ChristophWurst @skjnldsv Ready for review

ChristophWurst
ChristophWurst approved these changes Mar 21, 2018
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Nice

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 21, 2018
@MorrisJobke MorrisJobke merged commit be35b54 into master Mar 22, 2018
@MorrisJobke MorrisJobke deleted the cleanup-oc_json branch March 22, 2018 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@rullzer rullzer rullzer approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish technical debt
Projects
None yet
Milestone
Nextcloud 14
Development

Successfully merging this pull request may close these issues.
3 participants
@MorrisJobke @rullzer @ChristophWurst