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

Get all flashes #6

Merged
merged 3 commits into from Feb 24, 2018

Conversation

Projects
None yet
3 participants
@xtreamwayz
Member

xtreamwayz commented Jan 27, 2018

  • Are you creating a new feature?
    • Why is the new feature needed? What purpose does it serve?
    • Base your feature on the develop branch, and submit against that branch.
    • Add only one feature per pull request; split multiple features over multiple pull requests
    • Add tests for the new feature.
    • Add documentation for the new feature.

Sometimes you don't know which messages are added. FlashMessages::getFlashes() retrieves an array with all messages for the current request.

xtreamwayz added some commits Jan 27, 2018

* WILL NOT return values set in the current request via `flash()`.
*
* @param mixed $default Default value to return if no flash value exists.
* @return mixed

This comment has been minimized.

@webimpress

webimpress Jan 27, 2018

Contributor

mixed or array ? or mixed[]?

*
* WILL NOT return values set in the current request via `flash()`.
*
* @param mixed $default Default value to return if no flash value exists.

This comment has been minimized.

@webimpress

webimpress Jan 27, 2018

Contributor

Can't see any param defined in the function signature.

@@ -61,6 +61,19 @@ interface FlashMessagesInterface
*/
public function getFlash(string $key, $default = null);
/**
* Retrieve all flash values.

This comment has been minimized.

@webimpress

webimpress Jan 27, 2018

Contributor

I'm not a big fan of copying code into the documentation, then we need to maintain it in two places, and always remember to update it in the docs.... I know it was already here and you've just updated it 👍

* Retrieve all flash values.
*
* Will return all values was set in a previous request, or if `flashNow()`
* was called in this request with the same `$key`.

This comment has been minimized.

@webimpress

webimpress Jan 27, 2018

Contributor

It is not clear what is the $key.

* WILL NOT return values set in the current request via `flash()`.
*
* @param mixed $default Default value to return if no flash value exists.
* @return mixed

This comment has been minimized.

@webimpress

webimpress Jan 27, 2018

Contributor

The same comments here as in the docs

* WILL NOT return values set in the current request via `flash()`.
*
* @param mixed $default Default value to return if no flash value exists.
* @return mixed

This comment has been minimized.

@webimpress

webimpress Jan 27, 2018

Contributor

And here as well.

@@ -42,6 +42,10 @@ public function getFlash(string $key, $default = null)
{
}
public function getFlashes() : array
{
}

This comment has been minimized.

@webimpress

webimpress Jan 27, 2018

Contributor

This method definition is wrong, must return array, but it's void. Not sure where (if) it is used?

@vitorloureiro

This comment has been minimized.

vitorloureiro commented Feb 12, 2018

Hi @xtreamwayz
It is possible to merge this? i think this is essential for the flash messages middleware.

@xtreamwayz xtreamwayz requested a review from weierophinney Feb 12, 2018

@xtreamwayz

This comment has been minimized.

Member

xtreamwayz commented Feb 12, 2018

Yes, it's possible after a review from @weierophinney :)

*
* WILL NOT return values set in the current request via `flash()`.
*
* @return array

This comment has been minimized.

@webimpress

webimpress Feb 12, 2018

Contributor

Can be removed, as we have it already in method signature.

*
* WILL NOT return values set in the current request via `flash()`.
*
* @return array

This comment has been minimized.

@webimpress

webimpress Feb 12, 2018

Contributor

The same as above, can be removed.

@xtreamwayz xtreamwayz merged commit 9b3757c into zendframework:release-1.0.0 Feb 24, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 92.754%
Details

xtreamwayz added a commit that referenced this pull request Feb 24, 2018

xtreamwayz added a commit that referenced this pull request Feb 24, 2018

xtreamwayz added a commit that referenced this pull request Feb 24, 2018

@xtreamwayz xtreamwayz deleted the xtreamwayz:feature/get-all-flashes branch Feb 24, 2018

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