Update FlashMessenger.php #5759

Closed
wants to merge 10 commits into
from

Projects

None yet

9 participants

@diegobittencourt

Hello, the feature of rendering is fantastic, but you can not print the current message, this second method would make this work.

@diegobittencourt diegobittencourt Update FlashMessenger.php
Hello, the feature of rendering is fantastic, but you can not print the current message, this second method would make this work.
178f487
@blanchonvincent

@diegobittencourt please provide unit tests

@tfountain

Doesn't this duplicate functionality we have in the Flash Messenger view helper? http://framework.zend.com/manual/2.2/en/modules/zend.view.helpers.flash-messenger.html

@froschdesign
Member

@tfountain
getMessagesFromNamespace != getCurrentMessagesFromNamespace

@diegobittencourt

I am providing tests for the method I created.

@diegobittencourt diegobittencourt added a commit to diegobittencourt/zf2 that referenced this pull request Feb 7, 2014
@diegobittencourt diegobittencourt Tests for #5759 c27f7cd
@diegobittencourt diegobittencourt referenced this pull request Feb 7, 2014
Closed

Tests for #5759 #5798

@diegobittencourt

Tests -> #5759

Please if something is missing or incorrect, show me for me to get.

@samsonasik

you should just add a commit here, make git push to your branch, no need to create new PR, so #5798 can be closed :)

@diegobittencourt

Hello, I joined the 2 commits into one, I should do something else?
Note: I'm sorry for the inconvenience this attempt to contribute.

@samsonasik

travis build failure (https://travis-ci.org/zendframework/zf2/jobs/18593426), you need to fix it with php-cs-fixer :

php php-cs-fixer.phar fix /path/to/file.php --fixers=trailing_spaces

you can download php-cs-fixer from http://get.sensiolabs.org/php-cs-fixer.phar

@diegobittencourt

/path/to/file.php >>> "file" would be the file which I want to fix?

@diegobittencourt

Well, I adjusted all the problems and everything was accepted, but left me a question regarding the last commit:

It was so

  • $ This-> assertTrue ($ this-> plugin-> hasCurrentMessages ());
  • $ This-> assertTrue ($ this-> plugin-> hasCurrentInfoMessages ());
  • $ This-> assertTrue ($ this-> plugin-> hasCurrentSuccessMessages ());
  • $ This-> assertTrue ($ this-> plugin-> hasCurrentErrorMessages ());

And was

  • $ This-> assertFalse ($ this-> plugin-> hasCurrentMessages ());
  • $ This-> assertFalse ($ this-> plugin-> hasCurrentInfoMessages ());
  • $ This-> assertFalse ($ this-> plugin-> hasCurrentSuccessMessages ());
  • $ This-> assertFalse ($ this-> plugin-> hasCurrentErrorMessages ());

hasCurrentMessages is not available in plugin? Is that correct? My knowledge in zend framework is still small, I got doubts.

@Martin-P

No need for this pull request, because this functionality already exists.

The method Zend\View\Helper\FlashMessenger::__call() acts as a proxy for Zend\Mvc\Controller\Plugin\FlashMessenger. I already use this functionality to display the current messages.

In my layout.phtml I use:

<?php echo $this->render('layout/partial/messages'); ?>

In layout/partial/messages.phtml I use this code to map and merge the messages:

<?php
$namespaces = array(
    'default',
    'error',
    'info',
    'success',
    'warning',
);

$allMessages = array();
foreach ($namespaces as $namespace) {
    $currentMessages = $this->flashMessenger()->getCurrentMessagesFromNamespace($namespace);
    $this->flashMessenger()->clearCurrentMessagesFromNamespace($namespace);
    $allMessages[$namespace] = array_merge($currentMessages, $this->flashMessenger($namespace));
}

var_dump($allMessages);
@diegobittencourt

Forgive me, but yes there is the need of the Implemented functionality.

The method render of class FlashMessenger does not print the current messages.

@Martin-P

The method render of class FlashMessenger does not print the current messages.

Instead of creating a complete new method with lots of duplicate code I think a much cleaner solution would be to attach a flag to the view helper and act upon that flag:

$this->flashMessenger()->setUseCurrentMessages(true)->render();
@diegobittencourt

That is, in one way or another, does not agree with my implementation. :P

@diegobittencourt

I agree with this:

public function render($namespace = PluginFlashMessenger::NAMESPACE_DEFAULT, array $classes = array())
{
          $flashMessenger = $this->getPluginFlashMessenger();
          $messages = $flashMessenger->getMessagesFromNamespace($namespace);
          return renderAssistant($messages, $classes);
}
public function renderCurrent($namespace = PluginFlashMessenger::NAMESPACE_DEFAULT, array $classes = array())
{
          $flashMessenger = $this->getPluginFlashMessenger();
          $messages = $flashMessenger->getCurrentMessagesFromNamespace($namespace);
          return renderAssistant($messages, $classes);
}
protect function renderAssistant($messages, $classes)
{
...
}

But disagree on creating an attribute that would have no utility unless this method, taking into account the whole structure that FlashMessenger own class.

@Martin-P

Please use markdown code to make your code readable: https://help.github.com/articles/github-flavored-markdown

But disagree on creating an attribute that would have no utility unless this method, taking into account the whole structure that FlashMessenger own class.

Depends on what you want. I assumed you want to merge the messages, but I understand you want to be able to display the current messages separately?

I would suggest a bit more descriptive method name for renderAssistant like renderMessages. IMO the word Assistent assumes a separate class which assists the FlashMessenger view helper.

@diegobittencourt

Look at my last commit, maintains the structure and become clean. ;)

@Martin-P

Nice! Much cleaner now 😃 Now it's a matter of waiting until this PR is accepted.

@diegobittencourt

This is a question of mine, who performs this work?

@Martin-P

You can look at the closed PR's for this: https://github.com/zendframework/zf2/pulls?direction=desc&page=1&sort=created&state=closed

Some PR's are merged in 2 hours, but if I look at some of my PR's there has not been any response in 24 days, so I honostly got no idea how long it takes. I think they are somewhat behind of schedule looking at the fact there are still 160 PR's open.

@grizzm0
grizzm0 commented Feb 14, 2014

The who idea with FlashMessenger is to render something on the NEXT request. If you need to render the current message, doesn't that just mean you're using it wrong?

@Martin-P

If you need to render the current message, doesn't that just mean you're using it wrong?

The documented way of using it is indeed rendering messages on the next request. The FlashMessenger plugin however has a method getCurrentMessagesFromNamespace. What is the use of that method when messages should only be rendered at the next request? Personally I already used the FlashMessenger for displaying messages on the current request, because it has the fuctionality I need and I don't see why I have to reinvent the wheel.

What would be your suggestion to render messages on the current request?

@diegobittencourt

My changes in my opinion obeys the principles of FlashMessenger. No reinvention of the wheel. I await an opinion of the management as my changes.

@Maks3w
Member
Maks3w commented Feb 25, 2014

@diegobittencourt This PR conflicts with current master. Please rebase or merge master in the branch

@diegobittencourt

Is there any practice of documentation should I execute to perform this merge in my repository?

@samsonasik

@diegobittencourt you can do :

git checkout master
git pull git://github.com/zendframework/zf2.git 
git checkout patch-1
git rebase master

On this stage, maybe you will got conflict, so you need to fix conflict and git add the conflicted file :

git add /path/to/conflicted/file.php

and then git commit and then push --force to your branch

git commit -m "your commit message" -a
git push --force origin patch-1
@diegobittencourt

Good at my base I already have the last change, so I think it is all right :D

@samsonasik

when you did rebase and push --force, your commits must be showed under my last comment.... it seems you didn't do push --force ;)

@diegobittencourt

Look again my friend.

It seems that now with your tips will produce more and less mess.

@weierophinney weierophinney added this to the 2.3.0 milestone Mar 3, 2014
@weierophinney weierophinney self-assigned this Mar 3, 2014
@weierophinney weierophinney added a commit that referenced this pull request Mar 5, 2014
@weierophinney weierophinney [#5759] Added changelog note afbe7ae
@weierophinney weierophinney added a commit that referenced this pull request Mar 5, 2014
@weierophinney weierophinney Merge branch 'feature/5759' into develop
Close #5759
c246bca
@weierophinney
Member

Merged to develop for release in 2.3.0.

@weierophinney weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#5759 from diegobittenc…
…ourt/patch-1

Update FlashMessenger.php

Conflicts:
	library/Zend/View/Helper/FlashMessenger.php
701cbff
@weierophinney weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/5759' into develop 77607e1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment