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

Several fixes for usage with php 7.3 #9

Closed
wants to merge 6 commits into from

Conversation

thomaslauria
Copy link

  • Avoid error-logs full of PHP Warnings due to autoloads from class-exists()-checks
  • session errors
  • Fix Test class path

@Shardj
Copy link
Owner

Shardj commented Jul 24, 2019

Sounds like you need to fix your warnings, not suppress them, I can't really agree with that change to Loader.php

Could you explain the purpose of the changes to Session.php and what that helps fix? I'm not having any session issues with my build so I'm wondering what causes whatever the issue is.

The changes to DbTable's write function are fine, but the changes to destroy are fundamentally wrong, a query can fail for reasons other than the item already being deleted. You should check for existence before attempting to delete.

The changes to phpunit will cause issues for people who still use a version of PHPUnit that has PHPUnit_FrameworkTestCase. It'll also cause issues for people who have solved this issue with the following snippet which you can put in your bootstrap file class PHPUnit_Framework_TestCase extends \PHPUnit\Framework\TestCase {}.

@Shardj Shardj closed this Jul 24, 2019
@thomaslauria
Copy link
Author

Loader.php: due the usage of class_exist in the application we were getting a lot of warnings here. If a class really does not exist (and is even not loadable via the Loader) a warning was produced which is not needed at all. So suppressing the include warnings is OK here, since if the loader can not load something this produces a follow up error, either on usage of the class or resulting in a false result from class_exist.

Session fixes:
As far as I remember one of our main problems was the usage of session regeneration and / or the rememberUntil stuff. I will provide more details shortly.

  • to the change "$_sessionStarted instead sessionExists()" in rememberUntil: we run into a situation where sessionExists() returned false which gave an error with either the following session_get_cookie_params() or session_set_cookie_params(). In the $_sessionStarted variable the value was set correctly to the situation.

  • The change register_shutdown_function('session_write_close'); came also in the context of session rewriting, but I don't remember the exact error.

  • to the delete: I agree that the reason could be different errors, but I disagree to make a select before, since this would introduce race conditions. I think the real problem is, that delete() returns false if nothing was deleted (which is as explained in the code comment no error). It should return only false if there was an error.

PHPUnit:
I agree with your fix via extending - but since older phpunit versions are not usable with php7 - is this really a problem?

@Shardj
Copy link
Owner

Shardj commented Jul 26, 2019

  • The loader.php change makes sense after testing with Zend_Loader. That's all fine.
  • I'm not sure about the change from sessionExists() to $_sessionStarted, just because a session was started doesn't mean it exists, and the sessionExists function seems pretty competent. Do you know why sessionExists was failing for you? It may have been for good reason.
  • Session data is usually stored after your script terminated without the need to call session_write_close(), I just don't see the need for this? Again do you know why this was failing for you? https://php.net/manual/en/function.session-write-close.php
  • I can see what you mean about race conditions here, but returning false if nothing was deleted is still correct logically, if you don't care about if the delete actually deleted or not then why have a bool return in the first place, make it void. But I disagree with that, people may have functionality dependent on it returning false if nothing was found. Exceptions should be thrown if there's an error, not returning false.
  • This repo still supports from 5.2.11 onwards, not from 7 onwards, so the PHPUnit fix definitely can't go in.

develart-projects pushed a commit that referenced this pull request Oct 8, 2024
…large-text

restore mbstring overloading for full backwards compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants