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

[4.0] Make sure the function matches the parent return type #25261

Merged
merged 4 commits into from
Jun 18, 2019

Conversation

roland-d
Copy link
Contributor

@roland-d roland-d commented Jun 18, 2019

Summary of Changes

This fixes a fatal error when accessing the site

Fatal error: Declaration of Joomla\CMS\Session\Storage\JoomlaStorage::setOptions(array $options) must be compatible with Joomla\Session\Storage\NativeStorage::setOptions(array $options): Joomla\Session\Storage\NativeStorage in libraries/src/Session/Storage/JoomlaStorage.php on line 23

Testing Instructions

  1. Checkout the current Joomla 4 branch
  2. Open the site and you get this fatal error:
Fatal error: Declaration of Joomla\CMS\Session\Storage\JoomlaStorage::setOptions(array $options) must be compatible with Joomla\Session\Storage\NativeStorage::setOptions(array $options): Joomla\Session\Storage\NativeStorage in libraries/src/Session/Storage/JoomlaStorage.php on line 23
  1. Apply this pull request
  2. Run composer update
  3. Reload the site
  4. Site now loads

Expected result

The site loads

Actual result

The site doesn't load

Documentation Changes Required

None

Pinging @wilsonge

Signed-off-by: Roland Dalmulder <contact@rolandd.com>
Signed-off-by: Roland Dalmulder <contact@rolandd.com>
Signed-off-by: Roland Dalmulder <contact@rolandd.com>
Signed-off-by: Roland Dalmulder <contact@rolandd.com>
@mbabker
Copy link
Contributor

mbabker commented Jun 18, 2019

Also for clarity, on the Fatal error: A void function must not return a value in /usr/local/var/www/joomlacms/libraries/src/Session/Session.php on line 233 error...

Everything that's been typehinted is either new API for CMS 4.0 or Framework 2.0, or exists in a non-extendable scope (final class, private method, etc.). Anything that already exists in a stable CMS 3.x or Framework 1.x shouldn't be typehinted because of B/C breaks in changing signatures, unless the change was explicitly vetted (and there are some, but they're the exception to the rule). That one slipped in because we lost the @since 1.0 tag on the parent method in the Framework repo (it had @since __DEPLOY_VERSION__ so I figured it was new when going through things). So if you start running into type issues, first look for a B/C break then revert that break (unless explicitly vetted and OK'd as such) before trying to adapt code to the changed signature.

@richard67
Copy link
Member

@roland-d Is it ready for testing?

@roland-d
Copy link
Contributor Author

@mbabker Thanks for the input

@richard67 It should be ready now, I cleaned up the test instructions.

@wilsonge wilsonge merged commit ecfb522 into joomla:4.0-dev Jun 18, 2019
@wilsonge
Copy link
Contributor

Tested this locally and works. Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Jun 18, 2019
@richard67
Copy link
Member

@wilsonge Bad boy 👅 . I was so keen on testing it, and now you merged already 😄

@roland-d roland-d deleted the feature/session-clear-compatibility branch June 18, 2019 18:52
@wilsonge
Copy link
Contributor

sorrrrrrry :/

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

Successfully merging this pull request may close these issues.

None yet

5 participants