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

MessagesStorage & RequestStorage #4

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@MartinMajor

MartinMajor commented Apr 20, 2014

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.

@MartinMajor

This comment has been minimized.

MartinMajor commented Apr 20, 2014

See also related pull request.

@hrach

This comment has been minimized.

Contributor

hrach commented Apr 20, 2014

Awesome! 👍

* Checks if a flash session namespace exists.
* @return bool
*/
function isOpened();

This comment has been minimized.

@rostenkowski

rostenkowski Apr 20, 2014

AFAIK it should be isOpen()

This comment has been minimized.

@mishak87

mishak87 Apr 20, 2014

Actually should be hasMessages() anything else is misleading.

/**
* @author Filip Procházka
*/
class MessagesStorage extends Nette\Object implements IMessagesStorage

This comment has been minimized.

@rostenkowski

rostenkowski Apr 20, 2014

Maybe MessageStorage would be better... (avoid that double "sS") ... RequestStorage uses also singular...

This comment has been minimized.

@MartinMajor

MartinMajor Apr 20, 2014

I agree. What do you think @fprochazka?

This comment has been minimized.

@dg

dg May 4, 2014

Member

I agree too.

if (!isset($session[$key]) || ($session[$key][0] !== NULL && $session[$key][0] !== $this->user->getId())) {
return;
if ($response = $this->requestStorage->restoreRequest($key)) {
$this->sendResponse($response);

This comment has been minimized.

@rostenkowski
@rostenkowski

This comment has been minimized.

rostenkowski commented Apr 20, 2014

Great work! :)

/**
* Checks if a flash session namespace exists.

This comment has been minimized.

@mishak87

mishak87 Apr 20, 2014

Comment assumes implementation will use session.

This comment has been minimized.

@MartinMajor

MartinMajor Apr 20, 2014

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.

interface IMessagesStorage
{
const FLASH_KEY = '_fid';

This comment has been minimized.

@mishak87

mishak87 Apr 20, 2014

Has nothing to do with storage.

@@ -42,7 +42,7 @@
/** @internal special parameter key */
const SIGNAL_KEY = 'do',
ACTION_KEY = 'action',
FLASH_KEY = '_fid',
FLASH_KEY = Application\IMessagesStorage::FLASH_KEY,

This comment has been minimized.

@mishak87

mishak87 Apr 20, 2014

NO Custom implementation should piggyback on Presenter. Still you will need bridge between storage and presenter for translation of parameter and setup. If you really want to remove logic and dependency from presenter.

You are hiding dependency.

@mishak87

This comment has been minimized.

mishak87 commented Apr 20, 2014

What is the reason for refactoring two different functionalities in one pull?

@fprochazka

This comment has been minimized.

Contributor

fprochazka commented Apr 20, 2014

@mishak87 you should take a breath, calm down and think before you write.

@hrach

This comment has been minimized.

Contributor

hrach commented Apr 20, 2014

@mishak87 @rostenkowski I have already did a review, you are just copying my notes ;)

@MartinMajor

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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.

MartinMajor added some commits Apr 20, 2014

Presenter::hasFlashSession() & getFlashSession() are separated into M…
…essageStorage service (thanks to Filip Prochazka)
Presenter::storeRequest() & restoreRequest() are separated into Reque…
…stStorage service; restoreRequest() redirects to original URL
@MartinMajor

This comment has been minimized.

MartinMajor commented May 31, 2014

I've renamed MessagesStorage -> MessageStorage and MessagesStorage::isOpended() -> MessageStorage::hasMessages()

@MartinMajor

This comment has been minimized.

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

This comment has been minimized.

mishak87 commented May 31, 2014

@MartinMajor Name of the key is related to routing or presenter. Storage should not know about it.

@MartinMajor

This comment has been minimized.

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?

*/
public function getMessages($id = NULL)
{
return $this->getSession()->$id;

This comment has been minimized.

@dg

dg Jun 2, 2014

Member

Probably here should by (array)

@dg

This comment has been minimized.

Member

dg commented Jun 2, 2014

I think it is great. The only little bit confusing thing is mixing terms message vs flash.

@mishak87

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

dg commented Jun 3, 2014

Another thing: is it really needed to have REQUEST_KEY and FLASH_KEY in URL together?

@MartinMajor

This comment has been minimized.

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).

@dg

This comment has been minimized.

Member

dg commented Jul 1, 2014

I think we get rid of _rid, it should work with one identifier ;)

@dg dg referenced a pull request that will close this pull request Jul 1, 2014

Open

Pull requeststorage #17

@dg

This comment has been minimized.

Member

dg commented Jul 1, 2014

@MartinMajor what do you think about this solution?

@MartinMajor

This comment has been minimized.

MartinMajor commented Jul 2, 2014

I'll look at it till the end of this week (hopefully).

@MartinMajor

This comment has been minimized.

MartinMajor commented Jul 4, 2014

@dg I like your solution and it works great.

@f3l1x

This comment has been minimized.

Member

f3l1x commented Aug 9, 2015

👍

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