libreform / wp-libre-form

Easy native HTML5 forms for WordPress. Version 1.5 is unmaintained, but works without issue. 2.0 has been rewritten from the ground, and can be found at https://github.com/libreform/libreform
https://wordpress.org/plugins/wp-libre-form
GNU General Public License v3.0
67 stars 27 forks source link

why do you check if the php class exists? #87

Closed aoloe closed 6 years ago

aoloe commented 6 years ago

the php code is included with require_once, but each file contains the check

if ( ! class_exists( 'WP_Libre_Form' ) ) :

i think i've never seen such a check in php code...

k1sul1 commented 6 years ago

If you wanted to, you could define WP_Libre_Form again before it loads. That would cause a fatal error if it weren't for the check.

In reality, It's just a guard & good practice, don't worry about it.

aoloe commented 6 years ago

Can you please point me to the source that says it's a good practice?

k1sul1 commented 6 years ago

No. You can find hundreds of sources on the internet. It's called defensive programming.

There's literally no downsides to it, other than the two extra lines caused by the if-block.

aoloe commented 6 years ago

Ok, you're free to close this.

Just one final remark from my side. Skimming through the code, I spotted a few others things that make me wonder, why you're doing it.

Personally, I see a downside with your approach: not following some of the basic good practices (at those are things that I call good practice... like not relying on _once doing the right thing or heavily mixing html output and php logic) just makes potential contributors have a harder time getting into the project. Of course, you're free to stick to your habits/ideas: It's your project. but I don't agree that it has no downsides. (And yes, after having seen that if class_exists I was immediately more critical when browsing your code... It can happen so fast...)

So, your reply in this ticket, makes me refrain from trying to contribute to your project -- even if I love the idea behind it -- since there are too many things that "surprise" me. You might get some more tickets, but probably no code contributions.

Finally, I also see a technical downside in double checking the "onceity" of the include: including the class twice is probably the sign that the programmer is doing something wrong and I prefer PHP to fail early -- if possible during the development phase -- rather than hiding errors that could lead to crashes in production.

But as said: your project your choices!

k1sul1 commented 6 years ago

I closed the issue because I don't think this is an issue, and I hate open issues.

AFAIK we don't load anything in such a way that we'd ever have problems with our code defining multiple instances. It's a rather common practice in the WordPress community. It just might help someone else.

The guy who wrote most of the code is far more competent than I am, and if he thinks that class_exists is useful there, I'm not going to question on whether or not it should be there. It might not be necessary, but it certainly doesn't do any harm.

I don't agree with WordPress Coding Standards, but it was decided that this project uses them. I also don't agree with some things or ways this project does or uses, but as long as it gets the job done I don't really care, because none of us are getting paid for this, so we improve it chunk by chunk. There are issues with far higher priority, like we should probably utilize the Composer autoloader.

It's so simple I can just just glance at the source and I know how to achieve what I want, and my form markup renders just like I wanted it to, unlike with every other alternative I know of.

timiwahalahti commented 6 years ago

As one of the maintainers, I strongly agree with @k1sul1 here.

That being said, we also know that documentation could be better and it would help others to contribute (#86) and we're making efforts to achieve that. Personally I think the code is quite simple and there's not really much of it, so everyone used to WordPress (plugin development) should be able to get the idea.

Your free to send us a pull requests improving our code and this plugin, those will be much appreciated :)