Skip to content

Comments

Remove unused methods of OC_Json#8791

Merged
MorrisJobke merged 1 commit intomasterfrom
cleanup-oc_json
Mar 22, 2018
Merged

Remove unused methods of OC_Json#8791
MorrisJobke merged 1 commit intomasterfrom
cleanup-oc_json

Conversation

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Mar 12, 2018

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
if ($format == 'json') {
return OC_JSON::encode($response);
if (is_array($response)) {
array_walk_recursive($response, array('OC_JSON', 'to_string'));
Copy link
Member

@ChristophWurst ChristophWurst Mar 12, 2018

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

@rullzer rullzer Mar 12, 2018

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

@ChristophWurst ChristophWurst Mar 12, 2018

Choose a reason for hiding this comment

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

🤐

Copy link
Member Author

@MorrisJobke MorrisJobke Mar 12, 2018

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

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

@ChristophWurst ChristophWurst Mar 12, 2018

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

@MorrisJobke MorrisJobke Mar 12, 2018

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

MorrisJobke commented Mar 13, 2018

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

MorrisJobke commented Mar 21, 2018

@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

@rullzer rullzer rullzer approved these changes

@ChristophWurst ChristophWurst ChristophWurst 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