Skip to content

Comments

Request->getHeader() should always return a string#7813

Merged
MorrisJobke merged 1 commit intomasterfrom
getHeader-should-only-return-string
Jan 17, 2018
Merged

Request->getHeader() should always return a string#7813
MorrisJobke merged 1 commit intomasterfrom
getHeader-should-only-return-string

Conversation

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Jan 12, 2018

PHPDoc (of the public API) says that this method returns string but it also returns null, which is not allowed in some method calls. This fixes that behaviour and returns an empty string and fixes all code paths that explicitly checked for null to be still compliant.

Found while enabling the strict_typing for lib/private for the PHP7+ migration. (#7392)

The only app that uses this as well is activity and I will create a PR for that as well.

@MorrisJobke MorrisJobke added the 3. to review Waiting for reviews label Jan 12, 2018
@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone Jan 12, 2018
nickvergessen
nickvergessen reviewed Jan 12, 2018
View reviewed changes
lib/private/AppFramework/Http/Request.php
}

return null;
return '';
Copy link
Member

@nickvergessen nickvergessen Jan 12, 2018

Choose a reason for hiding this comment

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

Breaking public API :(

Copy link
Member Author

@MorrisJobke MorrisJobke Jan 12, 2018

Choose a reason for hiding this comment

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

Not really - the public API says: @return string

Copy link
Member Author

@MorrisJobke MorrisJobke Jan 12, 2018

Choose a reason for hiding this comment

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

Actually this fixes the public API, because you can see, that many apps use it like it will always return a string.

Copy link
Member

@nickvergessen nickvergessen Jan 12, 2018

Choose a reason for hiding this comment

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

apart from apps that checked the actual return instead of trusting the docs :D

Copy link
Member

@rullzer rullzer Jan 12, 2018

Choose a reason for hiding this comment

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

well we should communicate all this properly. But at some point we want to fix the code. I would vote to actually cleanup the code. It is not like we have a million apps that depend on all this.

Copy link
Member

@nickvergessen nickvergessen Jan 12, 2018

Choose a reason for hiding this comment

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

sure, I was just saying that "communicate" is the important word.

I'd really like to use @nextcloud/maintainers or something like that to mention such important cases.

Copy link
Member Author

@MorrisJobke MorrisJobke Jan 12, 2018

Choose a reason for hiding this comment

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

Let us collect this in a separate ticket for 14. Because I guess there will be more to come.

Copy link
Member Author

@MorrisJobke MorrisJobke Jan 12, 2018

Choose a reason for hiding this comment

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

#7827

This was referenced Jan 12, 2018
Merged
Closed
rullzer
rullzer previously requested changes Jan 15, 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.

test fail

@MorrisJobke MorrisJobke force-pushed the getHeader-should-only-return-string branch from b0bf116 to fc576cd Compare January 16, 2018 09:30
@codecov
Copy link

codecov bot commented Jan 16, 2018 •
edited
Loading

Codecov Report

Merging #7813 into master will increase coverage by <.01%.
The diff coverage is 84.61%.

@@             Coverage Diff              @@
##             master    #7813      +/-   ##
============================================
+ Coverage     51.23%   51.23%   +<.01%     
  Complexity    24919    24919              
============================================
  Files          1604     1604              
  Lines         94966    94969       +3     
  Branches       1376     1376              
============================================
+ Hits          48656    48659       +3     
  Misses        46310    46310
Impacted Files Coverage Δ Complexity Δ
apps/workflowengine/lib/Check/FileSize.php 0% <0%> (ø) 16 <0> (ø) ⬇️
lib/private/Memcache/APCu.php 79.59% <100%> (ø) 24 <0> (+2) ⬆️
lib/private/AppFramework/Http/Request.php 92.68% <100%> (-0.03%) 148 <0> (-1)
lib/private/Log/File.php 56.04% <100%> (+0.98%) 30 <0> (ø) ⬇️
lib/private/L10N/Factory.php 72.22% <100%> (ø) 72 <0> (ø) ⬇️
core/Controller/CssController.php 93.33% <100%> (ø) 7 <0> (-1) ⬇️
core/Controller/ClientFlowLoginController.php 79.35% <100%> (ø) 25 <0> (ø) ⬇️
core/Controller/JsController.php 93.33% <100%> (ø) 7 <0> (-1) ⬇️
lib/private/OCS/DiscoveryService.php 85.71% <0%> (-2.75%) 12% <0%> (+1%)
apps/files_trashbin/lib/Trashbin.php 72.53% <0%> (+0.24%) 136% <0%> (ø) ⬇️

rullzer
rullzer approved these changes Jan 16, 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.

🚀 lets do this!

@rullzer
Copy link
Member

rullzer commented Jan 16, 2018

still fail ;)

@MorrisJobke MorrisJobke self-assigned this Jan 16, 2018
@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 17, 2018
@MorrisJobke MorrisJobke force-pushed the getHeader-should-only-return-string branch 2 times, most recently from fe4ab92 to 90495c1 Compare January 17, 2018 08:49
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Jan 17, 2018
@MorrisJobke
Request->getHeader() should always return a string
4ef302c 
PHPDoc (of the public API) says that this method returns string but it also returns null, which is not allowed in some method calls. This fixes that behaviour and returns an empty string and fixes all code paths that explicitly checked for null to be still compliant.

Found while enabling the strict_typing for lib/private for the PHP7+ migration.

Signed-off-by: Morris Jobke <[email protected]>
@MorrisJobke MorrisJobke force-pushed the getHeader-should-only-return-string branch from 90495c1 to 4ef302c Compare January 17, 2018 08:51
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Jan 17, 2018

Fixed the tests.

@MorrisJobke MorrisJobke removed their assignment Jan 17, 2018
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Jan 17, 2018

Was basically the stuff in https://github.com/nextcloud/server/pull/7813/files#diff-9889f094c61189d26dace2f6e224459f

@MorrisJobke MorrisJobke merged commit c121610 into master Jan 17, 2018
@MorrisJobke MorrisJobke deleted the getHeader-should-only-return-string branch January 17, 2018 10:42
LEDfan added a commit to nextcloud/jsxc.nextcloud that referenced this pull request Mar 7, 2018
@LEDfan
Support strict typing in NC14 for IRequest::getHeader()
213e493 
(nextcloud/server/pull/7813)
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

@nickvergessen nickvergessen nickvergessen approved these changes

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish

Projects

None yet

Milestone

Nextcloud 14

Development

Successfully merging this pull request may close these issues.

3 participants

@MorrisJobke @rullzer @nickvergessen