Allow to mark JSON properties & XML nodes as optional #102

teklakct · 2017-07-27T22:28:29Z

What has been done

added optional expander

useless for scalar matchers

allows to mark JSON properties & XML nodes as optional

should resolve #93
optional() expander is also available for ArrayMatcher due to JsonMatcher & XmlMatcher is created using ArrayMatcher. There's no a simple way to exclude optional() expander functionality from ArrayMatcher

added test cases for MatcherTest
Matcher/Pattern/Pattern now have new method hasExpander(string $expanderName): bool

Other's

~~Also return type declaration was added for for Pattern inteface.~~

…nition

norberttech · 2017-08-01T19:32:08Z

src/Matcher/ArrayMatcher.php

+        return array_filter($notExistingKeys, function ($pattern) use ($values) {
+            try {
+                $typePattern = $this->parser->parse($pattern);
+            } catch (\Exception $e) {


we should not catch generic exception

You're right. What do you think about Coduo\PHPMatcher\Exception\Exception? When parsing pattern there could be thrown a UnknownExpanderException or UnknownExpanderException so we should catch them all.

catching Coduo\PHPMatcher\Exception\Exception sounds good

norberttech · 2017-08-01T19:35:24Z

src/Matcher/Pattern/TypePattern.php

+    public function hasExpander(string $expanderName)
+    {
+        foreach ($this->expanders as $expander) {
+            if (!$this->expanderInitializer->hasExpanderDefinition($expanderName)) {


wait, why do we need to use expanderInitializer? Can't we check if expander deos not exists in $this->expanders field?

I'm afraid not. Until hasExpander checks if expander exist using its name (simple string - key in $expanderDefinitions list) but in $this->expanders there is a expander instances. I think using expnaderInitializer is better than using some kind of regexp to check if the class name matches with $expanderName. These also should be some rules name new expanders.

I have tried, without success, not to used expanderInitializer. Do you have a better idea?

In that case I would rather like to expose expanderName from expander instance than inject expander initializer here

👍 ok, I'll try with that

Is there any easiest way to expand expander name than add getName to expander interface PatternExpander::getName()?

Adding getName sounds good

or you can add PatternExpander::is($name) instead of getName - that should fit better in this case

if we use constants for expander name , there is no need to use getName neither is($name) I'll revert changes to PatternExpander interface

there is because constant can't be a part of interface. Please add both is($name) and the constant

in this case, I propose to I add abstract Expander class with is($name). What do you think about that? Are there any disadvantages to using abstract expander?

teklakct · 2017-08-01T22:26:32Z

src/Matcher/ArrayMatcher.php

@@ -179,9 +180,13 @@ private function findNotExistingKeys($pattern, $values)
        $notExistingKeys = array_diff_key($pattern, $values);

        return array_filter($notExistingKeys, function ($pattern) use ($values) {
+            if (is_array($pattern)) {
+                return !$this->match($values, $pattern);


@norzechowicz I'm not pretty sure that we must check if values match to pattern because it already hasn't array_diff_key($pattern, $values) and also because OptionalExpander works only with top level match

norberttech · 2017-08-02T08:35:19Z

src/Matcher/Pattern/Expander/InArray.php

+     */
+    public function getName()
+    {
+        return 'inArray';


Could you please make constant from those names? For example:

InArray::NAME

This should allow to use constants here https://github.com/coduo/php-matcher/blob/master/src/Parser/ExpanderInitializer.php#L18

You can also replace "Coduo\\PHPMatcher\\Matcher\\Pattern\\Expander\\StartsWith" with StartsWith::class

norberttech · 2017-08-02T16:39:37Z

src/Matcher/Pattern/TypePattern.php

@@ -82,7 +82,7 @@ public function getError()
    public function hasExpander(string $expanderName)
    {
        foreach ($this->expanders as $expander) {
-            if ($expander->getName() === $expanderName) {
+            if ($expander::NAME === $expanderName) {


that's the problem, interface can't guarantee that Expander will have NAME constant. Please add is($name) method as I suggested in my previous comment

teklakct · 2017-08-02T16:59:02Z

@norzechowicz is this is what you mean?

* Add optional expander definition * Add hasExpander method for Pattern * Allow mark JSON properties & xml nodes as optional * Use intuitive matcher type definition in Xml test stock qty node definition * Add information about optional Expander to README * Remove return type declaration * Catch PHPMatcher Exception when parsing pattern for optional values * Add getName to PatternExpander interface * Remove unncessary ExpanderInitializer dependency in TypePattern * Use constants for expander name instead of method * Redeclare $expanderDefinitions using constants * Add static is($name) method to PatternExpander interface

* Allow to mark JSON properties & XML nodes as optional (#102) * Add optional expander definition * Add hasExpander method for Pattern * Allow mark JSON properties & xml nodes as optional * Use intuitive matcher type definition in Xml test stock qty node definition * Add information about optional Expander to README * Remove return type declaration * Catch PHPMatcher Exception when parsing pattern for optional values * Add getName to PatternExpander interface * Remove unncessary ExpanderInitializer dependency in TypePattern * Use constants for expander name instead of method * Redeclare $expanderDefinitions using constants * Add static is($name) method to PatternExpander interface * revert php 7.0 feature * drop php 5.3, 5.4 support * Use proper test case base class * Use proper test case base class * Fixed has expander method

teklakct added 6 commits July 27, 2017 20:52

Add optional expander definition

4c78197

Add hasExpander method for Pattern

d6ec56c

Allow mark JSON properties & xml nodes as optional

ff9ab1a

Use intuitive matcher type definition in Xml test stock qty node defi…

ef29d68

…nition

Add information about optional Expander to README

b780687

Remove return type declaration

3de0aab

teklakct mentioned this pull request Jul 27, 2017

Optional JSON fields #93

Closed

norberttech reviewed Aug 1, 2017

View reviewed changes

teklakct added 3 commits August 1, 2017 23:46

Catch PHPMatcher Exception when parsing pattern for optional values

c8e0045

Add getName to PatternExpander interface

12c60ce

Remove unncessary ExpanderInitializer dependency in TypePattern

a454c90

teklakct commented Aug 1, 2017

View reviewed changes

norberttech reviewed Aug 2, 2017

View reviewed changes

teklakct added 2 commits August 2, 2017 18:08

Use constants for expander name instead of method

1df7251

Redeclare $expanderDefinitions using constants

a50d669

norberttech reviewed Aug 2, 2017

View reviewed changes

Add static is($name) method to PatternExpander interface

1088d4f

norberttech merged commit 8cfa24b into coduo:master Aug 2, 2017

teklakct deleted the feature/optional-expander branch August 2, 2017 17:28

andylibrian mentioned this pull request Aug 28, 2017

Releasing "Optional" Json Fields in 2.x #106

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to mark JSON properties & XML nodes as optional #102

Allow to mark JSON properties & XML nodes as optional #102

teklakct commented Jul 27, 2017 •

edited

Loading

norberttech Aug 1, 2017

teklakct Aug 1, 2017

norberttech Aug 1, 2017

norberttech Aug 1, 2017

teklakct Aug 1, 2017 •

edited

Loading

norberttech Aug 1, 2017

teklakct Aug 1, 2017 •

edited

Loading

teklakct Aug 1, 2017

norberttech Aug 1, 2017

norberttech Aug 2, 2017

teklakct Aug 2, 2017

norberttech Aug 2, 2017

teklakct Aug 2, 2017

teklakct Aug 1, 2017

norberttech Aug 2, 2017

norberttech Aug 2, 2017

teklakct commented Aug 2, 2017

Allow to mark JSON properties & XML nodes as optional #102

Allow to mark JSON properties & XML nodes as optional #102

Conversation

teklakct commented Jul 27, 2017 • edited Loading

What has been done

Other's

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teklakct Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teklakct Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teklakct commented Aug 2, 2017

teklakct commented Jul 27, 2017 •

edited

Loading

teklakct Aug 1, 2017 •

edited

Loading

teklakct Aug 1, 2017 •

edited

Loading