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

Fix session errors and View errors() helper always empty #817

Merged
merged 5 commits into from
Oct 16, 2024

Conversation

circulon
Copy link
Contributor

@josephmancuso

Sorry I broke this PR badly and had to recreate it so we have lost the chat history ;(.

This PR addresses #816:

moves the MessageBag helper in the View from 'bag()'
back to its documented expected value 'errors()'
adds the "errors" key back into the session for backwards compatibility.

One thing I would ask is:

Is there any reason not to have the errors added back to the session as a MessageBag?
This makes SharedErrorsInSessionMiddleware a bit redundant...

Happy to discuss

will add Tests for SessionMiddleware soon

Added the errors key back into the session flash.
Changed the View errors helper name from ‘bag’  back to ‘errors’
Other modules will probably not expect a MessageBag as the content of the session ‘errors’ key.
So just pass it through
@circulon
Copy link
Contributor Author

circulon commented Aug 30, 2024

@josephmancuso
I think this is ready now ;)

Looks like this issue has been hiding since v4.16.4 around commit 594d50b

@circulon
Copy link
Contributor Author

circulon commented Sep 4, 2024

@josephmancuso @eaguad1337
could you review this so we can get a new release out with all the current fixes (including this one)

Cheers

@circulon
Copy link
Contributor Author

@josephmancuso LGTM please and tagged

This is impacting multiple areas of development.

Cheers

@josephmancuso
Copy link
Member

this is actually a pretty smart solution

@josephmancuso josephmancuso merged commit e12a938 into MasoniteFramework:4.0 Oct 16, 2024
6 checks passed
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.

2 participants