-
-
Notifications
You must be signed in to change notification settings - Fork 115
Comments
Conversation
MartinMajor
commented
Apr 20, 2014
See also related pull request.
hrach
commented
Apr 20, 2014
Awesome! 👍
src/Application/IMessagesStorage.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK it should be isOpen()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually should be hasMessages() anything else is misleading.
stepansvoboda
commented
Apr 20, 2014
Great work! :)
src/Application/IMessagesStorage.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment assumes implementation will use session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true and it should be changed. But storing flash messages into another storage than sessions is quite problematic, because you have to handle, that no one can see another user's messages. Sessions gives you this for free.
mishak87
commented
Apr 20, 2014
What is the reason for refactoring two different functionalities in one pull?
fprochazka
commented
Apr 20, 2014
@mishak87 you should take a breath, calm down and think before you write.
hrach
commented
Apr 20, 2014
@mishak87 @rostenkowski I have already did a review, you are just copying my notes ;)
MartinMajor
commented
Apr 20, 2014
@mishak87 My goal was to separate RequestStorage and provide redirect to original URL when restoreRequest() is called. But RequestStorage has dependency to flash messages so I had to separate them too.
mishak87
commented
Apr 20, 2014
@fprochazka Next time write what I did wrong instead of how you think I should handle myself which is extremely rude.
@MartinMajor Such note would be very helpful in the description (削除) along with review from @hrach (削除ここまで).
As Majkl always reminds people don't force push pulls it messes up comments.
fprochazka
commented
Apr 20, 2014
@mishak87 I think it's extremely rude to push people into doing work that is not neccesary. For example telling them to separate code to two pullrequests if it's already separated by commits.
FYI pullrequests are meant to be forcepushed in.
mishak87
commented
Apr 21, 2014
@fprochazka Nobody is rushing anyone. I asked a simple question and got an answer. Your little tantrums are OT.
What pull requests mean or not I do consult only The Spaghetti Monster.
...essageStorage service (thanks to Filip Prochazka)
...stStorage service; restoreRequest() redirects to original URL
MartinMajor
commented
May 31, 2014
I've renamed MessagesStorage -> MessageStorage and MessagesStorage::isOpended() -> MessageStorage::hasMessages()
MartinMajor
commented
May 31, 2014
I don't know what problem is with FLASH_KEY in IMessageStorage.
IMessageStorage stores flash messages into some storage but its key has to be transfered in URL no matter what storage is used. Other services (e.g. RequestStorage) has to be able get this key, so this constant should be IMHO in IMessageStorage.
FLASH_KEY in Presenter has to be for back compatibility.
mishak87
commented
May 31, 2014
@MartinMajor Name of the key is related to routing or presenter. Storage should not know about it.
MartinMajor
commented
Jun 1, 2014
@mishak87 And where would you put that constant? In Presenter? So all services, that need to use it (e.g. RequestStorage) had to have dependency on Presenter?
src/Application/MessageStorage.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably here should by (array)
dg
commented
Jun 2, 2014
I think it is great. The only little bit confusing thing is mixing terms message vs flash.
mishak87
commented
Jun 2, 2014
@MartinMajor To sum it up. I think this is step forward. But there is still missing what I would call bluntly layer for embedding non presenter related parameters into url (set of interfaces or events). If you try separating flash message functionality into own package you would see the issues.
IMO flash message is Web 1.0 concept and should be done away completely. It does not help with UX issues and offers only limiting functionality to user. User can't react on it. Some Nette projects modify its behavior, because they are lazy to write its own component. Or just for translating text. Final solution should enable creation of more featured replacements. Icons, links, buttons, templating etc.
dg
commented
Jun 3, 2014
Now I think that MessageStorage is over-engeneered :-) Because it is not about "messages" and it's doubled $id in setId() and setMessage() is confusing and very closely tighted to components in presenter.
In fact, MessageStorage is just storage for any values. It can become FlashStorage, (削除) but the fact that it exists only few seconds is not its main purpose (in addition it must be set manually via setExpiration). The main purpose is to be "attached" to the single tab in browser. So it is BrowserTabStorage. (削除ここまで)
And (削除) BrowserTabStorage (削除ここまで) FlashStorage, without all "message stuff", is universal storage, so it can be implemented in nette/http and used in application this way dg@9a59074.
This pull should therefore look like dg@0147db4
dg
commented
Jun 3, 2014
Another thing: is it really needed to have REQUEST_KEY and FLASH_KEY in URL together?
MartinMajor
commented
Jun 3, 2014
@dg: ok, as I wrote above, my main goal was to refactor RequestStorage, so I have nothing against TabStorage.
REQUEST_KEY & FLASH_KEY: When request is restored (and REQUEST_KEY is used) there can be generated new flash message (e.g. "login successful") so there are both this keys in URL at the same time (if that is what have you asked for).
3463b9a to
ee24f8e
Compare
ddf16b5 to
87e4597
Compare
c62f26c to
e72b735
Compare
f536135 to
4e8f41d
Compare
c478b71 to
44181e2
Compare
This pull request separates flash messages into new service MessagesStorage. It also separates store & restore requests into new service RequestStorage. This new service redirects to original URL when restoring old requests.