search

parisholley / wordpress-fantastic-elasticsearch

Improve wordpress search performance/accuracy and enable faceted search by leveraging an ElasticSearch server.
MIT License
162 stars 64 forks source link

Add default option to Config::option #49

Closed markoheijnen closed 11 years ago

markoheijnen commented 11 years ago

When retrieving 'enable_categories' you can receive null. In that case you get a warning since you are passing a non array value to in_array().

michaelsauter commented 11 years ago

The & definitely needs to be removed. You need a different setup to make your tests work.

parisholley commented 11 years ago

why does it need to be removed?

michaelsauter commented 11 years ago

If you have E_STRICT error reporting enabled, you'll get this when using &get_option(): Strict standards: Only variables should be assigned by reference in wp-content/plugins/fantastic-elasticsearch/src/elasticsearch/Config.php on line 23

You really should use the most verbose error reporting when developing to catch those sorts of things ...

parisholley commented 11 years ago

The unit tests are configured with STRICT but after playing with it a bit, it appears that is isn't being applied to the tests (i've gone as far as putting error_reporting and other things right before that would be thrown and it doesn't fail the test). As much as I agree that the code should be STRICT compatible, due to my time constraints I do not intend on going out of my way to fix them as it doesn't affect any of my environments. I've tried digging into PHPUnit and my bootstrap, and couldn't figure out why STRICT messages are not causing the tests to fail. If I put bad code in the boostrap.php it self, phpunit complains, but not when its within the scope of a test. If you come up with a way to make the tests fail, I'd be grateful! I don't intend on manually testing a app/instance/plugin for the sake of STRICTness.

michaelsauter commented 11 years ago

I do understand it's time consuming to fix this.

I'm not even sure that the unit tests should fail if you get warnings though. In any case, just relying on unit tests is never going to be good enough, I would always encourage to have at least one run through the actual application and see if it works as intended. I'm pretty sure you're often going to find something that you haven't accounted for in your tests (I have spotted a couple of those errors in your plugin already).

Also consider the following: I have enabled strict standards in my setup, and any plugin that cannot be changed with little effort to not produce any errors is not going to be used. I guess I'm not alone with this policy of "no plugins that throw warnings". The reasoning behind this policy is that it shows whether the plugin author cares enough to produce quality code and is following best practices ... if you start installing plugins that produce warnings or throw errors, eventually you'll end up with an unmaintainable system.

parisholley commented 11 years ago

I hear you on that, but i have unit tests, integration tests, and selenium tests that light up a VM and run through an instance with the plugin on. One of those three should catch ANY problems, including STRICT. If not, its an issue with how the test harness is setup. Again, I don't disagree where you are coming from, and I am sure a lot of people are as well, but at the end of the day, my time is limited. So if someone would like to help alter the test suites to catch these issues, they are free too.

Furthermore, people need to also take the time to fix tests when issuing pull requests, regardless of whether they agree with how the code is done.

parisholley commented 11 years ago

i merged your changes into 2.2.0 and altered the tests to make it work.

michaelsauter commented 11 years ago

Cool!