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.

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

Correctly skip suppressed errors in PHP 8.0 #27631

Merged
merged 1 commit into from
Jul 13, 2021
Merged

Correctly skip suppressed errors in PHP 8.0 #27631

merged 1 commit into from
Jul 13, 2021

Conversation

yan12125
Copy link
Contributor

Applies the suggested transformation mentioned in
https://www.php.net/manual/en/migration80.incompatible.php,

The @ operator will no longer silence fatal errors (E_ERROR,
E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_RECOVERABLE_ERROR,
E_PARSE). Error handlers that expect error_reporting to be 0 when
@ is used, should be adjusted to use a mask check instead

The new code still works on PHP 7, as error_reporting() already
returns 0 when diagnostics are suppressed.

This fixes #25807 in PHP 8.0.
For PHP 7.x, #22243 suppresses
the E_NOTICE message from the second session_start() call with the error
suppression operator @, and thus those E_NOTICE messages are still
logged in PHP 8.0.

See also #25806

Signed-off-by: Chih-Hsuan Yen yan12125@gmail.com

@yan12125
Copy link
Contributor Author

As a side note: other error_reporting() calls in Nextcloud already uses the mask pattern

lib/private/Log/ErrorHandler.php:		if (!(error_reporting() & $number)) {
lib/base.php:			error_reporting(E_ALL);
apps/files/lib/Command/Scan.php:		if (!(error_reporting() & $severity)) {
apps/files/lib/Command/Scan.php:			// This error code is not included in error_reporting
apps/files/lib/Command/ScanAppData.php:		if (!(error_reporting() & $severity)) {
apps/files/lib/Command/ScanAppData.php:			// This error code is not included in error_reporting

@szaimen szaimen added this to the Nextcloud 22 milestone Jun 23, 2021
@szaimen
Copy link
Contributor

szaimen commented Jun 23, 2021

cc @skjnldsv, @eneiluj, @icewind1991 and @ArtificialOwl for feedback on this

@szaimen szaimen added the 3. to review Waiting for reviews label Jun 23, 2021
@yan12125
Copy link
Contributor Author

Hmm I'm not sure if I understand the failed test. Drone CI [1] reports that "And I see that the new user form is shown" failed, while the new user form seems fine with my patched installation.

[1] https://drone.nextcloud.com/nextcloud/server/6543/62/4

@blizzz blizzz mentioned this pull request Jun 23, 2021
39 tasks
@blizzz blizzz modified the milestones: Nextcloud 22, Nextcloud 23 Jun 24, 2021
@yan12125
Copy link
Contributor Author

After rebasing onto the latest git-master, the acceptance-login test becomes green but sqlite and integration-sharing-v1-video-verification turn red. I think those are unstable tests like ones listed in #22305

@AndyXheli
Copy link
Contributor

Tested on NC 22 and looks good.

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Makes sense 👍

@szaimen
Copy link
Contributor

szaimen commented Jul 7, 2021

Not sure if this is important:

There was 1 failure:
407 |  
408 | 1) Test\Files\Cache\UpdaterLegacyTest::testWrite
409 | Failed asserting that 1624596572 is equal to 1624596573 or is greater than 1624596573.
410 |  
411 | /drone/src/tests/lib/Files/Cache/UpdaterLegacyTest.php:118

@yan12125
Copy link
Contributor Author

yan12125 commented Jul 7, 2021

Checking again, the failure in UpdaterLegacyTest.php is mentioned as an unstable test at #22305 (comment)

@yan12125
Copy link
Contributor Author

yan12125 commented Jul 9, 2021

Hmm, how can I proceed here? GitHub refuses to merge this PR as some tests failed.

Also, could this be backported to versions that declare PHP 8 support (stable-22 and stable-21)?

@skjnldsv

This comment has been minimized.

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 9, 2021
@skjnldsv
Copy link
Member

skjnldsv commented Jul 9, 2021

Can you rebase to master please @yan12125 ?

@yan12125
Copy link
Contributor Author

yan12125 commented Jul 9, 2021

Can you rebase to master please @yan12125 ?

Done! Previous failures are green this time, but there is a new failure, and it is not mentioned in #22305

  Scenario: users navigation without disabled users                                   # /drone/src/tests/acceptance/features/users.feature:48
    Given I act as Jane                                                               # ActorContext::iActAs()
    And I am logged in as the admin                                                   # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I open the User settings                                                      # SettingsMenuContext::iOpenTheUserSettings()
    And I open the "Disabled users" section                                           # AppNavigationContext::iOpenTheSection()
    And I see that the list of users contains the user disabledUser                   # UsersSettingsContext::iSeeThatTheListOfUsersContainsTheUser()
      The user disabledUser in the list of users is not shown yet after 100 seconds
    And I open the actions menu for the user disabledUser                             # UsersSettingsContext::iOpenTheActionsMenuOf()
    And I see that the "Enable user" action in the disabledUser actions menu is shown # UsersSettingsContext::iSeeTheAction()
    When I click the "Enable user" action in the disabledUser actions menu            # UsersSettingsContext::iClickTheAction()
    Then I see that the section "Disabled users" is not shown                         # AppNavigationContext::iSeeThatTheSectionIsNotShown()
    When I open the User settings                                                     # SettingsMenuContext::iOpenTheUserSettings()
    Then I see that the section "Disabled users" is not shown                         # AppNavigationContext::iSeeThatTheSectionIsNotShown()

@nickvergessen
Copy link
Member

That's okay and unrelated I think

@yan12125
Copy link
Contributor Author

yan12125 commented Jul 9, 2021

Thanks! But GitHub still insists on green for all tests 😂

Applies the suggested transformation mentioned in
https://www.php.net/manual/en/migration80.incompatible.php,

> The @ operator will no longer silence fatal errors (E_ERROR,
> E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_RECOVERABLE_ERROR,
> E_PARSE). Error handlers that expect error_reporting to be 0 when
> @ is used, should be adjusted to use a mask check instead

The new code still works on PHP 7, as error_reporting() already
returns 0 when diagnostics are suppressed.

This fixes #25807 in PHP 8.0.
For PHP 7.x, #22243 suppresses
the E_NOTICE message from the second session_start() call with the error
suppression operator @, and thus those E_NOTICE messages are still
logged in PHP 8.0.

See also #25806

Signed-off-by: Chih-Hsuan Yen <yan12125@gmail.com>
@juliushaertl juliushaertl merged commit e1f644a into nextcloud:master Jul 13, 2021
@juliushaertl
Copy link
Member

/backport to stable22

@juliushaertl
Copy link
Member

/backport to stable21

@yan12125 yan12125 deleted the php8-fix-error-reporting branch July 13, 2021 07:25
@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@yan12125
Copy link
Contributor Author

Hmm strange. Created manual backports #27938 and #27939. Hopefully I'm doing it right :)

@juliushaertl
Copy link
Member

Looks good :) Thanks for already taking care of that 👍

@yan12125
Copy link
Contributor Author

Hmm is this needed on stable20? The latter does not declare support for PHP 8.0 [1]. I guess it will not bring errors, though.

[1] https://github.com/nextcloud/server/blob/stable20/lib/versioncheck.php

@MichaIng
Copy link
Member

Ah that is true, if fails even when overriding the check, due to broader incompatibility. The bot is sleeping anyway 😄.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nextcloud 21 - Error: session_start(): Ignoring session_start() because a session is already active
9 participants