Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Settings & more #112

Merged
merged 20 commits into from Nov 8, 2018
Merged

Implement Settings & more #112

merged 20 commits into from Nov 8, 2018

Conversation

dugajean
Copy link

@dugajean dugajean commented Nov 6, 2018

What's Included in this Pull Request

  • First and foremost: Adjusts the project's directory organization into a more intuitive structure.
  • Implements PSR-2 and Inpsyde Coding Standards
  • Implements a class for handling options/settings (main feature)
  • Cache support added to the SettingsRepository (partial implementation of WP_Cache inclusion)
  • PHPUnit tests for SettingsRepository
  • Tab feature refactoring

Please note that this Pull Request is larger than it should be as it includes more than a specific feature. I deemed it as necessary to restructure the plugin's layout into something more intuitive and friendly. If something's out of order, feel free to let me know and I'll fix it.

Things not included in this Pull Request

  • Proper form display and handling (should each Tab have its own key in the mw_adminimize option or should there be only one large form including all settings which are split in tabs?)
  • Real forms. Current forms in the Pull Request are just dummy forms with no real meaning

@dugajean dugajean changed the title Refactor settings Implement Settings & more Nov 6, 2018
@dugajean
Copy link
Author

dugajean commented Nov 6, 2018

I didn't notice that the build has failed. It's because of an include in tests/bootstrap.php. I'm going to look into this and push a fix if possible.

@bueltge
Copy link
Collaborator

bueltge commented Nov 7, 2018

Thanks for this great PR. ❤️ I will check and validate your changes, ideas in the next view days. So that you have the chance to send a commit for the error check to fix them and the PR is more valid.

@bueltge
Copy link
Collaborator

bueltge commented Nov 8, 2018

@dugajean again and again - thanks a lot, great PR, nice improvements and I like them and I will merge it fastly. But I will wait for an commit to fix the travis build.

@dugajean
Copy link
Author

dugajean commented Nov 8, 2018

@bueltge Thanks a lot! I will be fixing that and push it very soon. I'm glad it's proper.

@bueltge
Copy link
Collaborator

bueltge commented Nov 8, 2018

@dugajean We have also an xml config file for PhpStorm for our php codex; maybe you need them? I will upload to the repository branch so that PhpStorm will use them. Makes easier to format via PhpStorm.

@bueltge
Copy link
Collaborator

bueltge commented Nov 8, 2018

Added via e8bf9dd

@dugajean
Copy link
Author

dugajean commented Nov 8, 2018

@bueltge Yes, that would be useful! Thank you!

So for the builds, everything should be fine now normally as the tests are passing and phpcs is returning only warnings, but the exit code isn't 0 and I finally figured out why:

I copied the Request and ParameterBag classes from the Inpsyde Google Tag Manager plugin and there are some phpcs suppressions there. Oddly, the php_codesniffer package doesn't take into consideration that the suppression means ignoring the error. I will adjust the .travis.yml config and see how it goes. Hopefully the builds will pass after that.

@bueltge bueltge merged commit 0a90cd7 into wp-media:refactor Nov 8, 2018
@bueltge
Copy link
Collaborator

bueltge commented Nov 8, 2018

That's a great step forward for the project. Maybe we find more time to go the next steps to leave the old plugin, the old code and get solid plugin with code there we will be maintain. Thanks again!

@bueltge
Copy link
Collaborator

bueltge commented Nov 8, 2018

After composer update I can't run locally the unittest.

/v/w/w/Adminimize (refactor) $ vendor/bin/phpunit 
PHPUnit 6.3.1 by Sebastian Bergmann and contributors.


<p>Dummy Content for AdminBar Settings</p>

PHP Fatal error:  Uncaught Error: Call to undefined function wp_kses() in /var/www/wp-plugins/Adminimize/src/Templates/AdminBar.php:5
Stack trace:
#0 /var/www/wp-plugins/Adminimize/vendor/phpunit/php-code-coverage/src/CodeCoverage.php(1122): include_once()
#1 /var/www/wp-plugins/Adminimize/vendor/phpunit/php-code-coverage/src/CodeCoverage.php(269): SebastianBergmann\CodeCoverage\CodeCoverage->initializeData()
#2 /var/www/wp-plugins/Adminimize/vendor/phpunit/phpunit/src/Framework/TestResult.php(659): SebastianBergmann\CodeCoverage\CodeCoverage->start(Object(Adminimize\Tests\Unit\AdminimizeTest))
#3 /var/www/wp-plugins/Adminimize/vendor/phpunit/phpunit/src/Framework/TestCase.php(883): PHPUnit\Framework\TestResult->run(Object(Adminimize\Tests\Unit\AdminimizeTest))
#4 /var/www/wp-plugins/Adminimize/vendor/phpunit/phpunit/src/Framework/TestSuite.php(744): PHPUnit\Framework\TestCase->run(Object(PHPUnit\Framework\TestResult))
#5 /var/www/wp-plugins/Adminimize/vendor/phpunit/phpunit/src/Framework/TestSuite.php(744): PHPUnit\Framework\ in /var/www/wp-plugins/Adminimize/src/Templates/AdminBar.php on line 5

Maybe you have a hint for me?
Also the tabs are currently not in WP default style and ux:
image

That' the default
image

image

@dugajean
Copy link
Author

dugajean commented Nov 8, 2018

@bueltge For the error, I can't seem to be able to reproduce it. Maybe it's because we don't have the same environments? I have PHP 7.2.9 and WordPress 4.9.8. I have used a missing function in Templates/AdminBar.php and the tests were passing anyway. Could you tell me what exactly you are using (describe your environment)?

I'll fix the tab design too. After I fix everything, I'll send another Pull Request.

@dugajean
Copy link
Author

dugajean commented Nov 9, 2018

After careful inspection I noticed that this error shows up when XDebug is on (it was off in my case). I'm going to look into a fix and push it.

Tabs have already been fixed. Pull Request pending...


So the issue happens because of the code coverage which happens when XDebug is running. I attempted mocking wp_kses, which fixed the problem, but then another error came up saying that CodeCoverage::form() doesn't exist (because of the $this->form() call in AdminBar.php).

This problem can be "fixed" by adding <directory>src/Templates</directory> under the <exclude> block in phpunit.xml.dist. I'm not sure if that's an acceptable solution though.

@bueltge
Copy link
Collaborator

bueltge commented Nov 9, 2018

I think we should not test via UnitTests the templates, so that we can exclude them.
Thanks for this analyze and your help!

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

Successfully merging this pull request may close these issues.

None yet

2 participants