Skip to content

Move require_once out of functions? #304

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Move require_once out of functions? #304

wants to merge 1 commit into from

Conversation

andyleap
Copy link

It's really a minor issue, but having require_once 'Expressions.php'; inside build_insert() and apply_where_conditions($args) of SQLBuilder.php seems a little odd, as that requires a stat of Expressions.php everytime those functions are called. It's a sub-microsecond optimization, but my framework is already running in the sub-microseconds for most requests. I don't see anything that'd be harmed by letting the autoload handle loading it, or even just moving it to the top of the file, so it's always loaded, but I'm still a little new to the guts of phpActiveRecord.

@al-the-x
Copy link
Collaborator

You're right, there were just some... issues with the autoloader that were recently resolved. Do you mind attaching some code to this issue or opening a separate pull request...?

@andyleap
Copy link
Author

Just a note, this works fine for my situation, been testing for a while, but I'm a bit abnormal of a use case, as I don't use the autoloader. It should be fine with just the autoloader, and the travis ci build tests support that, so I'm probably going to remove the lines completely and add that

@al-the-x
Copy link
Collaborator

@andyleap can you rebase and push to get us a happier merge? Wish Github had that feature already...

@andyleap
Copy link
Author

what do you mean by happier, just a single commit?

@koenpunt
Copy link
Collaborator

What about the other require_once statements in functions, like in Model.php#L620 and Table.php#L459 ?

@al-the-x
Copy link
Collaborator

@andyleap Instead of a collated (recursive) merge, a fast-forward merge with the Travis CI tests running.

@al-the-x al-the-x mentioned this pull request Jul 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants