Process multiple namespaces for class names and configuration#25
Process multiple namespaces for class names and configuration#25Ocramius merged 13 commits intolaminas:1.8.xfrom kliker02:at/namespace-feature-bc
Conversation
Signed-off-by: Aleksandr <laker.tv@bk.ru>
…config or actual in this version - multidimensional array config
Signed-off-by: Aleksandr <laker.tv@bk.ru>
| protected $pluginFlashMessenger; | ||
|
|
||
| /** @var array<string, FlashMessengerNamespace> */ | ||
| protected $namespaces = []; |
There was a problem hiding this comment.
Since this is newly introduced, let's make it private to avoid abuse
There was a problem hiding this comment.
Let's briefly document (in the docblock) that the keys are the namespace names
| * @return FlashMessenger | ||
| */ | ||
| public function setMessageCloseString($messageCloseString) | ||
| public function setMessageCloseString($messageCloseString, string $namespace = 'default') |
There was a problem hiding this comment.
We should probably have the string 'default' in a private const, to prevent copy-paste mistakes, and ease future refactoring
There was a problem hiding this comment.
As I've understood 'default' is a namespace name from Laminas\Mvc\Plugin\FlashMessenger\FlashMessenge::NAMESPACE_DEFAULT
| $objNamespace = $this->getNamespace($namespace); | ||
| if ($objNamespace !== null) { | ||
| return $objNamespace->getClasses(); | ||
| } else { |
There was a problem hiding this comment.
else not needed here.
I suggest we remove the negation though:
if ($objNamespace === null) {
return $this->classMessages[$namespace] ?? '';
}
return $objNamespace->getClasses();|
|
||
| private function getClasses(string $namespace): string | ||
| { | ||
| $objNamespace = $this->getNamespace($namespace); |
There was a problem hiding this comment.
To avoid $objNamespace (which is hungarian notation), let's instead use string $namespaceName as input parameter, and then just $namespace for the object form?
| { | ||
| $this->messageOpenFormat = (string) $messageOpenFormat; | ||
| $objNamespace = $this->getNamespace($namespace); | ||
| if ($objNamespace !== null) { |
There was a problem hiding this comment.
Similar to other locations in the patch:
elsecan be omitted when redundant- write the positive conditional first, rather than adding a negation (reduces mental overhead when reading)
| /** @var string|array<string,string|array> $property */ | ||
| foreach ($configHelper as $property) { | ||
| if (is_array($property)) { | ||
| $isArrayOneDimensial = false; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's move this to a private function
|
BTW, overall much better solution than previous approach! Nicely done, @kliker02! |
Signed-off-by: Aleksandr <laker.tv@bk.ru>
Ocramius
left a comment
Ocramius
left a comment
There was a problem hiding this comment.
LGTM. Would need @froschdesign to provide feedback on how to document this new feature though, since that should be part of this patch
Signed-off-by: Aleksandr <laker.tv@bk.ru>
Signed-off-by: Aleksandr <laker.tv@bk.ru>
Ocramius
changed the title
4 participants
Description
New feature, in accordance with the issue https://github.com/laminas/laminas-mvc-plugin-flashmessenger/issues/3
Fixes #3