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

Extend native SessionHandlerInterface #357

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

holtkamp
Copy link

@holtkamp holtkamp commented Jul 12, 2023

See https://www.php.net/manual/en/class.sessionhandlerinterface.php

This change allows classes that implement Zend_Session_SaveHandler_Interface to use typed properties.

This also makes it easier to start migrating to Laminas\Session

Functionally, nothing changes.

@holtkamp holtkamp closed this Jul 12, 2023
@holtkamp holtkamp deleted the patch-2 branch July 12, 2023 11:06
@holtkamp holtkamp restored the patch-2 branch July 12, 2023 11:06
@holtkamp holtkamp reopened this Jul 12, 2023
@hungtrinh
Copy link

@glensc could you please review this PR!

@develart-projects
Copy link
Collaborator

@holtkamp correct me if I'm wrong, but this is not backward-compatible change. Once merged, people could experience problem with wrong type even if they are not really using strict types.

@develart-projects develart-projects added the question Further information is requested label Aug 9, 2023
@holtkamp
Copy link
Author

holtkamp commented Aug 9, 2023

@develart-projects well, it depends on how strict the implementation of the interface is.

One can use a "loose" implementation which does not use typed parameters, for example:

interface Zend_Session_SaveHandler_Interface extends \SessionHandlerInterface {
	//empty on purpose
};

class MyLooseSessionHandler implements Zend_Session_SaveHandler_Interface {
    public function close(): bool
    {
        return true;
    }

    public function destroy($id): bool
    {
        return true;
    }

    public function gc($max_lifetime): int|false
    {
        return true;
    }

    public function open($path, $name): bool
    {
        return true;
    }

    public function read($id): string|false
    {
        return true;
    }

    public function write($id, $data): bool
    {
        return true;
    }
}

Or even looser without return types:

interface Zend_Session_SaveHandler_Interface extends \SessionHandlerInterface {
	//empty on purpose
};

class MyLooserSessionHandler implements Zend_Session_SaveHandler_Interface {
    public function close()
    {
        return true;
    }

    public function destroy($id)
    {
        return true;
    }

    public function gc($max_lifetime)
    {
        return true;
    }

    public function open($path, $name)
    {
        return true;
    }

    public function read($id)
    {
        return true;
    }

    public function write($id, $data)
    {
        return true;
    }
}

This is typically the current implementation for the Zend Framework, for example: https://github.com/Shardj/zf1-future/blob/master/library/Zend/Session/SaveHandler/DbTable.php

But one can also choose to use strict(er) implementation which does use typed parameters:

interface Zend_Session_SaveHandler_Interface extends \SessionHandlerInterface {
	//empty on purpose
};

class MyStrictSessionHandler implements Zend_Session_SaveHandler_Interface {
    public function close(): bool
    {
        return true;
    }

    public function destroy(string $id): bool
    {
        return true;
    }

    public function gc(int $max_lifetime): int|false
    {
        return true;
    }

    public function open(string $path, string $name): bool
    {
        return true;
    }

    public function read(string $id): string|false
    {
        return true;
    }

    public function write(string $id, string $data): bool
    {
        return true;
    }
}

All three example implementations are valid implementations for PHP.

Also the current Zend_Session_SaveHandler_DbTable remains a valid implementation, all checks pass...

Also see https://onlinephp.io/c/ba50d to play around...

Maybe I am missing something, then apologies for the time wasted, but I thought this was an elegant way to gradually migrate to Laminas\Session

@develart-projects
Copy link
Collaborator

@holtkamp the "looser" implementation is what I meant, generating shitload of deprecations (later warnings, later errors). But I think this will take short time to fix. Just don't like this forced strict types shift.

I'm actually using that, so I'll end up with deprecations waterfall here.

@develart-projects develart-projects added the enhancement New feature or request label Aug 9, 2023
@develart-projects develart-projects added this to the 1.23.0 milestone Aug 9, 2023
@develart-projects develart-projects merged commit de90587 into Shardj:master Aug 9, 2023
4 checks passed
@holtkamp
Copy link
Author

holtkamp commented Aug 9, 2023

@develart-projects yeah with the "looser" implementation deprecation messages like this will be generated:

Deprecated: Return type of MyLooserSessionHandler::open(string $path, string $name) should either be compatible with SessionHandlerInterface::open(string $path, string $name): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

Adding the return types to the implementation should resolve this.

But no run-time errors will / should occur...

Anyhow, thanks!

@develart-projects
Copy link
Collaborator

Allright, here we go, excatly as I predicted: #377

I'll most probably revert this change, because it's not backward compatibile and has no benefits, just screwing up existing code.

fanlei added a commit to fanlei/zf1-future that referenced this pull request Sep 13, 2023
fanlei added a commit to fanlei/zf1-future that referenced this pull request Sep 13, 2023
develart-projects added a commit that referenced this pull request Sep 13, 2023
…ecated

Fixed #357 return type backward-compatible issue reported at #377
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants