Skip to content
This repository has been archived by the owner on Mar 27, 2018. It is now read-only.

Hoa shouldn't initialize before being used #95

Closed
jchamberlain opened this issue Oct 5, 2015 · 7 comments
Closed

Hoa shouldn't initialize before being used #95

jchamberlain opened this issue Oct 5, 2015 · 7 comments

Comments

@jchamberlain
Copy link

I'm including hoa/math in my project so that I can evaluate arithmetic expressions (which is great, btw!). Unfortunately, this means Hoa's Core.php automatically initializes, unexpectedly polluting the global namespace and running code that can easily throw errors if run at the wrong time. Examples:

  • A couple dozen constants are defined across multiple files in the global namespace. None have even a vendor prefix.
  • Global configuration is changed without warning (i.e, mb_internal_encoding()).
  • Global functions _define() and event() are defined, meaning I can't define them for my own project. I only realized this when I tried calling Laravel's event() function.
  • date is called, so if your php.ini doesn't have a timezone set, you get an error.

Because of your composer.json, all of this happens before any of my project's code has a chance to run, so the fact that Laravel defines an event() function doesn't matter, nor does its setting the timezone prevent Hoa from throwing an error.

Could you put your constants in a class and your functions in Hoa's namespace? Then you could also do the initialization in the constructor and remove Core.php from the autoload "files" array. 99% of request to my app don't require Hoa, so I don't expect it to initialize until its needed.

@jubianchi
Copy link
Member

@jchamberlain I know the way the Core is initialized is not optimal for every use-cases but here are some thnigs you can test:

  • in the file where you require the composer autloader (i.e vendor/autoload.php) just require your own bootstrap (or the one from Laravel) before composer's autoloader so that your function will be defined first
  • Hoa/Core will always check if functions exist before defining them (example here)
  • Calling mb_*_encoding seems good if you are planning to use mbstring. Doing so you ensure the encoding is exactly what you expect it to be. Again, if you need to use a different value you can override this by calling mb_*_encoding after the require 'vendor/autoload.php';
  • setting the timezone in PHP is, to me, a prerequisite to run applications on a server.

99% of request to my app don't require Hoa, so I don't expect it to initialize until its needed.

Initializing Hoa is a matter of some miliseconds I don't think you will even spot the difference when removing it. @Hywan could you give us some numbers here ?

@jchamberlain
Copy link
Author

Thanks for your response.

  • Laravel relies on Composer (and uses the files array to load its own helpers), so I can't get it to run before Hoa. (Unless there's a way to change dependency order in Composer?)
  • Hoa, presumably because it's lower in the alphabet than Laravel, defines its functions first, so checking function_exists() doesn't help.
  • I don't have any complaints with mb_internal_encoding(), but rather that a dependency is unexpectedly and without asking changing PHP's configuration.
  • I agree that setting the timezone in PHP is a prerequisite, but setting it in php.ini is not. There are good reasons to use date_timezone_set(), and that very well may happen after dependencies are loaded.
  • My point about 99% is less about performance and more to highlight why I'm finding Hoa's initialization procedure so unexpected and unnecessary. If it didn't initialize until needed, none of this would be a problem because my functions would all be defined and configuration all entered (e.g., timezone).

Obviously I don't know Hoa well, so maybe its design is a necessity, but since I just need a way to evaluate user-supplied arithmetic I don't expect it to spill over into my other code like this. Is there any way it can be more self-contained and stick to its own namespace?

@Hywan
Copy link
Member

Hywan commented Oct 7, 2015

Few years ago, having Hoa\Core as a small set of files were logical and better for performances. Today, this is no longer the case (PHP is faster, OS I/O is faster, HDD is faster etc.). I am currently splitting Hoa\Core into Hoa\Event, Hoa\Consistency etc. and just one or two of them will be required all the time (like Hoa\Protocol —for hoa://— and Hoa\Consistency —for consistency— for instance).

About the conflict between Laravel and Hoa… I am annoyed. Yes Hoa should not declare global constants and functions, that's why we are using _defined and function_exists before each declaration. So our responsibility is safe. Now, Laravel should do the same thing. Also, these helpers (because they are always only aliases/helpers) can disappear in a short time. They are no longer useful anymore. They are here for historical reasons. We are going to drop PHP5.4 and it will introduce a BC break (by nature) so we could drop these helpers in the same time.

Thoughts?

@jubianchi
Copy link
Member

👍 with @Hywan seems a good plan :)

@jchamberlain
Copy link
Author

@hwyan, that sounds good!

@Hywan Hywan closed this as completed Oct 13, 2015
@Hywan Hywan removed the in progress label Oct 13, 2015
@Hywan
Copy link
Member

Hywan commented Oct 13, 2015

@jchamberlain Please, see #96.

@Hywan
Copy link
Member

Hywan commented Jan 12, 2016

Problem solved since Hoa\Core has been split.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants