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

Little Error and Improvement ideas in /lib/Compiler.class.php #54

Open
meitzner-hannes opened this issue Mar 3, 2014 · 5 comments
Open

Comments

@meitzner-hannes
Copy link
Collaborator

If I am not wrong the Second Line in the Compiler.class.php is useless.

<?php
$compiler = 'lessphp';
if( defined('WP_LESS_COMPILER') ){

The $compiler variable is never used again, is it?

Secondly, would it not been better to register a filter hook to let programmers change the folder with the compiler class base, instead of using globals?

Also would it not been better to encapsulate the checksequence for the includepath inside some other class? This plugin is nicely done in OOP but that look outside the norm.

I just used to switch from lessphp to less.php 'cause lessphp does not compile the "Semantic Grid System" less files right.
Was nicely surprised that switching the compiler class was soo easy! :)
Many thank!

@thom4parisot
Copy link
Owner

The $compiler variable is never used again, is it?

Yes it's not even used. Avoiding globals is a good practice indeed.

Secondly, would it not been better to register a filter hook to let programmers change the folder with the compiler class base, instead of using globals?

Also would it not been better to encapsulate the checksequence for the includepath inside some other class? This plugin is nicely done in OOP but that look outside the norm.

Yes, and ideally it should be done during the plugin configuration, not in the class file itself.
And providing a filter is a good stuff, a constant is just an easy way hence a bit overwhelming the wp-config.php file.

Feel free to propose a PR to fix those issues :-)

@meitzner-hannes
Copy link
Collaborator Author

Never done before but maybe its a good opportunity to look into that stuff ;)

@thom4parisot
Copy link
Owner

Never done before but maybe its a good opportunity to look into that stuff ;)

Yep don't worry, nobody will bite you and that's a valuable thing to learn :-)

@fabrizim
Copy link
Collaborator

fabrizim commented Mar 4, 2014

Yeah - thanks for catching that. The additional variable was my bad.

I do think that a filter for the compiler should be in the plugin initialization as well - but one issue is that the plugin object is created right away, and the compiler is instantiated in that constructor. I think we should hold off until the 'init' hook to create the compiler object - that would give themes / plugins a chance to hook into whatever filter we create to change the compiler.

@thom4parisot
Copy link
Owner

Good idea :-)

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

No branches or pull requests

3 participants