Skip to content
This repository was archived by the owner on May 1, 2019. It is now read-only.

[WIP] Feature: Flash Messenger Error Messages #421

Merged
merged 25 commits into from
Mar 14, 2015

Conversation

ins0
Copy link
Contributor

@ins0 ins0 commented Feb 24, 2015

This PR

  • Replaces the generic error exceptions with specific exception constructors
  • Extend the exceptions with a public message
  • Covers all flashmessenger messages in a clean bs message format
  • Covers the error view in a clean bs message format + added public messages from exceptions
  • Covers the 404 error in a clean bs message format

@ins0 ins0 force-pushed the feature/error-messages branch from 6b7a105 to 7b97bc8 Compare February 24, 2015 15:30
{
$this->classOptions = [
PluginFlashMessenger::NAMESPACE_INFO => [
'name' => $this->getTranslator()->translate('Information'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of translate calls that aren't actually needed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, avoid calling getTranslator everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius should i avoid using translate in order that at some point the site gets multi language or calling the translator once into a var and translate it into the loop?

@ins0
Copy link
Contributor Author

ins0 commented Feb 25, 2015

@Ocramius would be nice if you could review this like meat through a meat chipper 🍖

// or everyone else of course :)

$messageClasses = [
$namespace => $this->classOptions[PluginFlashMessenger::NAMESPACE_DEFAULT],
];
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid this else: do this assign operation earlier

@Ocramius
Copy link
Member

@ins0 done


class FlashMessenger extends ZendFlashMessenger
{
/** @var array */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string[][]

@ins0
Copy link
Contributor Author

ins0 commented Feb 25, 2015

thanks @Ocramius for your first review, changed most of the suggestions - others waiting for reply 😸

@ins0 ins0 force-pushed the feature/error-messages branch from e4c616a to 61ebb03 Compare February 27, 2015 20:37
@@ -211,7 +208,8 @@ public function removeAction()

$request = $this->getRequest();
if (!$request->isPost()) {
throw new Exception\UnexpectedValueException('Something went wrong with the post values of the request...');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw InvalidRequestDataException::fromInvalidPostRequest($request)

@ins0 ins0 force-pushed the feature/error-messages branch from 85a59be to b7676ff Compare March 6, 2015 13:48
@ins0
Copy link
Contributor Author

ins0 commented Mar 8, 2015

@Ocramius may take the final look over this, please? 😃

private $publicMessage;

/**
* @inheritdoc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ins0

Here!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah will fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@ins0
Copy link
Contributor Author

ins0 commented Mar 14, 2015

ping @Ocramius :)

@Ocramius
Copy link
Member

👍

Ocramius added a commit that referenced this pull request Mar 14, 2015
[WIP] Feature: Flash Messenger Error Messages
@Ocramius Ocramius merged commit 80e336d into zendframework:master Mar 14, 2015
@ins0 ins0 deleted the feature/error-messages branch March 14, 2015 21:48
@ins0
Copy link
Contributor Author

ins0 commented Mar 14, 2015

thanks @Ocramius @localheinz 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants