Skip to content

feat: make Session iterable #11

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

Merged
merged 4 commits into from
Dec 28, 2022
Merged

Conversation

yani
Copy link
Contributor

@yani yani commented Dec 24, 2022

Implemented \Iterator interface into Session class.
The \Iterator interface is what makes foreach() work.
Also added a testing scenario.

Closes my own issue: #10

Still wondering if Serializable is worth implementing.

Copy link
Owner

@compwright compwright left a comment

Choose a reason for hiding this comment

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

Well done! Thanks for including the test case. We have some test failures, I think it's from the missing guard statements.

@compwright compwright added the enhancement New feature or request label Dec 24, 2022
@compwright
Copy link
Owner

We don't need Serializable, there's nothing in Session that would require special handling. It's only needed when serializing objects that contain things needing special handling like resources. We have a simple array, and so Session can be serialized just fine as-is.

@compwright compwright changed the title implement Iterator in Session class feat: make Session iterable Dec 24, 2022
@yani
Copy link
Contributor Author

yani commented Dec 26, 2022

Error: Parameter #1 $array of function reset expects array|object, array|null given.
Error: Parameter #1 $array of function current expects array|object, array|null given.
Error: Parameter #1 $array of function key expects array|object, array|null given.
Error: Parameter #1 $array of function next expects array|object, array|null given.
Error: Parameter #1 $array of function key expects array|object, array|null given.

Reason is array|null on $contents:

    /**
     * @var mixed[]|null
     */
    protected ?array $contents = null;

I think array|object doesn't break anything so I'll change it to this and adjust accordingly:

protected array $contents = [];

@yani
Copy link
Contributor Author

yani commented Dec 26, 2022

By using empty arrays instead of null, isInitialized() might be obsolete.

Should I keep the method, and use a property like: protected bool $is_initialized = false;?

That way no changes to the Manager need to be done as it uses the method to determine whether or not to open the Session. I'm wondering if the Manager always opens the Session regardless of flow. If so, it would be better to remove the method.

Another method would be to simply ignore the errors.

@compwright
Copy link
Owner

I think array|object doesn't break anything so I'll change it to this and adjust accordingly:

protected array $contents = [];

Let's not do that, it's uninitialized for a reason.

Error: Parameter #1 $array of function key expects array|object, array|null given.

Reason is array|null on $contents

The guard statement will prevent acting on a null $contents, but PHPStan doesn't know that, so in this case I think it's fine to just add // @phpstan-ignore-next-line or whatever to suppress the error.

@yani
Copy link
Contributor Author

yani commented Dec 28, 2022

Updated tests as well

  Scenario: Iterate over populated session # features\access.feature:30
    When data exists                       # Compwright\PhpSession\BehaviorTest\AccessContext::dataExists()
    Then data is iterated                  # Compwright\PhpSession\BehaviorTest\AccessContext::iteratorSucceeds()

  Scenario: Iterate over non-populated session # features\access.feature:33
    When data does not exist                   # Compwright\PhpSession\BehaviorTest\AccessContext::dataDoesNotExist()
    Then data is not iterated                  # Compwright\PhpSession\BehaviorTest\AccessContext::iteratorFails()

@compwright compwright merged commit 723a911 into compwright:master Dec 28, 2022
compwright added a commit that referenced this pull request Dec 29, 2022
* implement Iterator in Session class

* make Session Iterator throw if not initialized

Co-authored-by: Jonathon Hill <[email protected]>

* phpstan: ignore iterator parameter errors in Session class

* update iterator tests

Co-authored-by: Yani <[email protected]>
Co-authored-by: Jonathon Hill <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants