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

[stable11] Treat PHP Errors on User session regenerate #5190

Merged
merged 3 commits into from Jun 15, 2017

Conversation

Projects
None yet
6 participants
@blizzz
Member

blizzz commented May 31, 2017

There are code paths that could disable apps (>= PHP 7)

fyi @LukasReschke

Treat PHP Errors on User session regenerate
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

@blizzz blizzz added this to the Nextcloud 11.0.4 milestone May 31, 2017

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot May 31, 2017

@blizzz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @icewind1991, @LukasReschke and @ChristophWurst to be potential reviewers.

mention-bot commented May 31, 2017

@blizzz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @icewind1991, @LukasReschke and @ChristophWurst to be potential reviewers.

remove unnecessary lines…
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz May 31, 2017

Member

Perhaps related: phpredis/phpredis#1062 despite redis module 3.1.2 is in use

Member

blizzz commented May 31, 2017

Perhaps related: phpredis/phpredis#1062 despite redis module 3.1.2 is in use

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Jun 1, 2017

Member

This works here, but i would rework the whole class and wrap any session_* call into an try…catch(Error) to consistently have a similar experience to PHP 5.6 times. I will create new PRs and close this once they are done.

Member

blizzz commented Jun 1, 2017

This works here, but i would rework the whole class and wrap any session_* call into an try…catch(Error) to consistently have a similar experience to PHP 5.6 times. I will create new PRs and close this once they are done.

change PHP errors to ErrorException in the session (PHP >=7)
Otherwise it might be that authentication apps are being disabled on
during operation while in fact the session handler has hiccup.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Jun 14, 2017

Codecov Report

Merging #5190 into stable11 will increase coverage by <.01%.
The diff coverage is 0%.

@@              Coverage Diff               @@
##             stable11    #5190      +/-   ##
==============================================
+ Coverage       57.41%   57.41%   +<.01%     
- Complexity      19445    19446       +1     
==============================================
  Files            1214     1214              
  Lines           72584    72585       +1     
  Branches         1237     1237              
==============================================
+ Hits            41677    41678       +1     
  Misses          30907    30907
Impacted Files Coverage Δ Complexity Δ
lib/private/Session/Internal.php 0% <0%> (ø) 20 <2> (+1) ⬆️
lib/private/Security/CertificateManager.php 92.78% <0%> (-1.04%) 38% <0%> (ø)
lib/private/Server.php 92.74% <0%> (+0.16%) 119% <0%> (ø) ⬇️
lib/private/Files/Storage/DAV.php 7.32% <0%> (+0.23%) 159% <0%> (ø) ⬇️
apps/dav/lib/CalDAV/PublicCalendar.php 100% <0%> (+7.14%) 10% <0%> (ø) ⬇️

codecov bot commented Jun 14, 2017

Codecov Report

Merging #5190 into stable11 will increase coverage by <.01%.
The diff coverage is 0%.

@@              Coverage Diff               @@
##             stable11    #5190      +/-   ##
==============================================
+ Coverage       57.41%   57.41%   +<.01%     
- Complexity      19445    19446       +1     
==============================================
  Files            1214     1214              
  Lines           72584    72585       +1     
  Branches         1237     1237              
==============================================
+ Hits            41677    41678       +1     
  Misses          30907    30907
Impacted Files Coverage Δ Complexity Δ
lib/private/Session/Internal.php 0% <0%> (ø) 20 <2> (+1) ⬆️
lib/private/Security/CertificateManager.php 92.78% <0%> (-1.04%) 38% <0%> (ø)
lib/private/Server.php 92.74% <0%> (+0.16%) 119% <0%> (ø) ⬇️
lib/private/Files/Storage/DAV.php 7.32% <0%> (+0.23%) 159% <0%> (ø) ⬇️
apps/dav/lib/CalDAV/PublicCalendar.php 100% <0%> (+7.14%) 10% <0%> (ø) ⬇️

code changed since

@blizzz blizzz added 3. to review and removed 2. developing labels Jun 14, 2017

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Jun 14, 2017

Member

I throw the session_* calls now into the invoker that does the error transformation.

Member

blizzz commented Jun 14, 2017

I throw the session_* calls now into the invoker that does the error transformation.

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Jun 14, 2017

Member

Needs forward ports to 12 and master @karlitschek

Member

blizzz commented Jun 14, 2017

Needs forward ports to 12 and master @karlitschek

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Jun 14, 2017

Member

👍 makes sense

Member

karlitschek commented Jun 14, 2017

👍 makes sense

@LukasReschke

I guess we have to do that then. Nice catch!

@MorrisJobke MorrisJobke merged commit 3e9cda3 into stable11 Jun 15, 2017

1 of 2 checks passed

continuous-integration/drone/push the build failed
Details
continuous-integration/drone/pr the build was successful
Details

@MorrisJobke MorrisJobke deleted the treat-session-regid-error-stable11 branch Jun 15, 2017

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Jun 15, 2017

Member

@blizzz Could you please do the forward ports? thanks

Member

MorrisJobke commented Jun 15, 2017

@blizzz Could you please do the forward ports? thanks

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Jun 15, 2017

Member

Thx for merging - will do!

Member

blizzz commented Jun 15, 2017

Thx for merging - will do!

blizzz added a commit that referenced this pull request Jun 15, 2017

Forward port of #5190 to stable12
Treat PHP Errors on User session regenerate

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

remove unnecessary lines…

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

change PHP errors to ErrorException in the session (PHP >=7)

Otherwise it might be that authentication apps are being disabled on
during operation while in fact the session handler has hiccup.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz added a commit that referenced this pull request Jun 15, 2017

Forward port of #5190 to master
Treat PHP Errors on User session regenerate

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

remove unnecessary lines…

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

change PHP errors to ErrorException in the session (PHP >=7)

Otherwise it might be that authentication apps are being disabled on
during operation while in fact the session handler has hiccup.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Jun 15, 2017

Member

Done → #5421 & #5422

Member

blizzz commented Jun 15, 2017

Done → #5421 & #5422

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment