Fixes 7: Fix defaults #13

thomscode · 2017-11-09T17:52:01Z

Fixes issue #7

Uses Dflydev\DotAccessData\Data to solve the problem.
Creates getDefaults() and setDefaults($defaults) methods to keep backwards compatibility
Adds TODO comments for steps to enforce $defaults class parameter being a Dflydev\DotAccessData\Data object and not an array in version 2.0 (however this breaks backwards compatibility.
Updates appropriate tests to ensure new functionality is tested.

greg-1-anderson · 2017-11-09T18:28:03Z

+1 LGTM. I just want to do a bit of testing with Robo to ensure there's no b/c problem, but implementation looks like it should be compatible.

thomscode · 2017-11-09T20:55:42Z

My initial robo command to test the default functionality and showed it wasn't working now works properly. I initially made a modification that I didn't realize was part of Robo not Config, which is why I found the backward compatibility issue. I'm planning on submitting a PR to Robo to update it's config class so it uses Dflydev\DotAccessData\Data instead of an array.

But I did make sure my PR didn't break the current functionality. :)

…ter. This allows sub-classes to ensure $defaults remains the proper type, while not providing additional functionality to classes using this class.

thomscode · 2017-11-12T18:47:01Z

Just updated visibility of the $default getter/setter methods. This makes them the same as the visibility of $default so it doesn't add any additional functionality to classes using this class, while allowing sub-classes to use the methods to ensure $default remains a Data object and not an array.

Better define type returned in an attempt to fix the Scrutinizer error.

thomscode · 2017-11-13T16:38:06Z

I'm done making changes now.

greg-1-anderson · 2017-11-13T17:46:32Z

src/Config.php

     */
    protected function getDefaults()
    {
        // Ensure $this->defaults is a Data object (not an array)
        if (is_array($this->defaults)) {
            $this->setDefaults($this->defaults);
+            return $this->getDefaults();
+        } elseif ($this->defaults instanceof Data) {


Minor style point: avoid elseif when if will do. (if block above exits when true.)

greg-1-anderson · 2017-11-13T17:46:57Z

src/Config.php

+            return $this->getDefaults();
+        } elseif ($this->defaults instanceof Data) {
+            return $this->defaults;
+        } else {


Minor style point: avoid else / extra indentation when unnecessary.

I completely agree with both of these points and had a version working that didn't have either the elseif or the else. The problem is that Scrutinizer said there was a "major" error introduced because it couldn't detect that $defaults wasn't an array, and array|Dflydev\DotAccessData\Data didn't match the return type of Dflydev\DotAccessData\Data.

I would prefer the simpler version of the code, but also don't want to introduce problems that don't go away with the code checking that's in place.

Your recommendation for the final solution?

I don't know if there is a solution to convince Scrutinizer that the return values are correct here, but it is permissible to ignore Scrutinizer inspection results if they are wrong.

My previous code just checked if $this->defaults was an array, and if so set it using $this->setDefaults($this->defaults) then returned $this->defaults (now set as a Data object).

This is what was causing problems in Scrutinizer.

As a solution to not using elseif and else but still solving the Scrutinizer issue, I could use an if in place of the elseif and simply throw and \Exception() without the else, because both if statements return if they are true.

Correct?

Yes, that's what I was advocating -- if in place of elseif. No change to logic of the function, only to the representation, to make it easier to read.

Reverts commits: * 5de0012 * f1f7cce * 1b03e3d

thomscode · 2017-11-20T22:09:30Z

Found this: https://scrutinizer-ci.com/docs/reviews/filtering_issues_and_false_positives

So I'm reverting unnecessary logic and going to attempt that. Hopefully that keeps the problem out of further Scrutinizer reviews down the road.

thomscode · 2017-11-20T22:13:38Z

The issue is back in Scrutinizer. I don't have permission to dismiss it (should have thought of that), but hopefully the instructions in the link above work for you and you are able to ignore the issue.

Either way the code is back to the point prior to my attempt to fix that problem.

greg-1-anderson · 2017-11-21T00:18:23Z

My experience with the Scrutinizer "false positive" feature is that it never remembers false positives, and continues to report the issue regardless of your stated preferences.

It is not necessary (nor desirable) to change the code to work around Scrutinizer advice that is incorrect.

…g as covered in code coverage report

thomscode · 2017-11-21T01:55:45Z

Updated to fix code coverage for protected methods.

Done making changes. Ready for merge per your manual tests.

thomscode · 2017-11-28T21:36:20Z

Any update on the testing this?

greg-1-anderson · 2017-11-28T21:41:49Z

src/Config.php

@@ -124,6 +126,7 @@ public function setDefault($key, $value)
     * TODO: remove Data object validation in 2.0
     *
     * @return Data
+     * @codeCoverageIgnore


You shouldn't @codeCoverageIgnore here. There is no reason why this shouldn't be covered; if it isn't, that fact should be reflected in the stats and coverage logs.

Would you rather just have this uncovered? or do you have a recommendation for how to test this? My research shows that private/protected methods should only be covered by what can be tested publicly. Since these methods are only for use to ensure the $defaults property is a Data object and not an array, they aren't called by any public methods.

I did try subclassing the method to initially create the $defaults property as an array, and test using that class, but since that appeared to only test the subclass and didn't change the code coverage for the actual Config class.

Unless I'm missing the right way to do this (totally possible 😕 )

It's possible to test private / protected methods by using php reflection to set the access level to public, and then call the method indirectly through the reflection object. I do this in some tests (not necessarily in this project, though). That said, I am a fan of testing the just the public methods of a class. If the protected / private methods become really complex and need their own direct testing, that's generally a sign that they should be factored out into another class.

Also, if a protected or private method exists to serve any useful function, then they should be reachable by some public methods, if called with the right input values. If they are never called, that might be an indication that they can be completely removed. Some functions just happen to not be called in tests, e.g. public function getFoo() { return $this->foo; } does not necessarily require a test.

These methods look simple enough, and I'm okay with leaving them untested.

Ok, thanks. I'll remove the @codeCoverageIgnore directives. I think the only part that wasn't getting tested was the parts that validate the $defaults property is not an array. I'll try creating a test using reflection that tests this by modifying the $defaults property directly and then calls the method.

I don't think that you need to use reflection just to call a protected method with bad values to ensure that the sanity checks work. This is testing the specific implementation over the behavior. Either figure out how to cover that code block through the public methods, or leave it untested.

I understand your concern with testing implementation instead of behavior, in this case I think it is important to test since these methods are what prevent breaking backward compatibility.

Also in creating the test I found a flaw that I would not have otherwise readily noticed. 😀

Thanks for merging and for the experience. I have learned a lot through this PR.

greg-1-anderson · 2017-11-28T21:42:44Z

Sort of backlogged due to the holidays. Will try to get to this shortly.

greg-1-anderson · 2017-11-28T23:16:58Z

This looks all good. Just remove the @codeCoverageIgnore and I'll merge it.

This forces any non-Data objects to use setter, and uses the Exception in the setter.

thomscode · 2017-11-28T23:58:19Z

Thanks for that tip on using reflection. Code coverage for the Config class is now back to 100% 😄

thomscode added 3 commits November 9, 2017 09:07

Change how defaults are handled by using Dflydev\DotAccessData

ccc5c1d

Add getters and setters to prevent breaking backwards compatibility

dde7b70

Update test to ensure partial defaults can be loaded

25d678c

thomscode added 2 commits November 12, 2017 10:42

Change visibility of $default getter/setter to match the class parame…

fd20ee8

…ter. This allows sub-classes to ensure $defaults remains the proper type, while not providing additional functionality to classes using this class.

Add description of parameter to docblock

2c4ccc9

thomscode added 2 commits November 12, 2017 10:52

Attempt to fix Scrutinizer complaint

5de0012

Update getDefaults method to better specify type

f1f7cce

Better define type returned in an attempt to fix the Scrutinizer error.

greg-1-anderson reviewed Nov 13, 2017

View reviewed changes

thomscode added 2 commits November 13, 2017 15:41

Fix code styling of if/elseif/else

1b03e3d

Reverts commits made to fix Scrutinizer false positive message

58975c7

Reverts commits: * 5de0012 * f1f7cce * 1b03e3d

Add codeCoverageIgnore attribute for protected methods not registerin…

78e545f

…g as covered in code coverage report

greg-1-anderson reviewed Nov 28, 2017

View reviewed changes

greg-1-anderson added a commit to consolidation/robo that referenced this pull request Nov 28, 2017

Try consolidation/config#13

8975bd7

greg-1-anderson mentioned this pull request Nov 28, 2017

Try 'fix-defaults' PR in consolidation/config consolidation/robo#649

Closed

4 tasks

thomscode added 3 commits November 28, 2017 15:50

Remove codeCoverageIgnore statements

69f435e

Force Config::setDefaults if Config::defaults is not a Data object

4f14e71

This forces any non-Data objects to use setter, and uses the Exception in the setter.

Add new test for Config::defaults protected getter/setter

b2ca3f4

greg-1-anderson merged commit c22cec9 into consolidation:master Nov 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes 7: Fix defaults #13

Fixes 7: Fix defaults #13

thomscode commented Nov 9, 2017

greg-1-anderson commented Nov 9, 2017

thomscode commented Nov 9, 2017

thomscode commented Nov 12, 2017

thomscode commented Nov 13, 2017

greg-1-anderson Nov 13, 2017

greg-1-anderson Nov 13, 2017

thomscode Nov 13, 2017

greg-1-anderson Nov 13, 2017

thomscode Nov 13, 2017

greg-1-anderson Nov 13, 2017

thomscode Nov 13, 2017

thomscode commented Nov 20, 2017

thomscode commented Nov 20, 2017

greg-1-anderson commented Nov 21, 2017

thomscode commented Nov 21, 2017

thomscode commented Nov 28, 2017

greg-1-anderson Nov 28, 2017

thomscode Nov 28, 2017

thomscode Nov 28, 2017

greg-1-anderson Nov 28, 2017

thomscode Nov 28, 2017

greg-1-anderson Nov 28, 2017

thomscode Nov 29, 2017

greg-1-anderson commented Nov 28, 2017

greg-1-anderson commented Nov 28, 2017

thomscode commented Nov 28, 2017

Fixes 7: Fix defaults #13

Fixes 7: Fix defaults #13

Conversation

thomscode commented Nov 9, 2017

greg-1-anderson commented Nov 9, 2017

thomscode commented Nov 9, 2017

thomscode commented Nov 12, 2017

thomscode commented Nov 13, 2017

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

thomscode commented Nov 20, 2017

thomscode commented Nov 20, 2017

greg-1-anderson commented Nov 21, 2017

thomscode commented Nov 21, 2017

thomscode commented Nov 28, 2017

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

greg-1-anderson commented Nov 28, 2017

greg-1-anderson commented Nov 28, 2017

thomscode commented Nov 28, 2017