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

No implicit construction in base.php #41

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sgpinkus
Copy link
Contributor

One liner. rm Base::instance() on load construction. Bad practice. Causes issues. Had pushed this earlier, but accidently deleted PR branch.

@sgtlambda
Copy link
Contributor

I'm all for this since it is indeed really bad practice. It should be noted, though, that currently in the README.md the following example is given:

$f3 = require('path/to/base.php');

(at https://github.com/bcosca/fatfree/tree/dev#hello-world-the-less-than-a-minute-fat-free-recipe)

@Rayne
Copy link
Member

Rayne commented May 4, 2015

This pull request breaks the backwards compatibility. I'm not expecting that @bcosca will drop this feature, so it would be a good idea to collect the issues.

I haven't experienced any issues although I am using composer and Base::instance() to get the instance.

Edit: Issues like (more ore less unexpected) environment modifications, e.g. overwriting the default locale, timezone, encoding, …?

@KOTRET
Copy link
Contributor

KOTRET commented May 4, 2015

it will break backwards compatibility for those who are doing $f3 = require('base.php'); and easy to fix. Beside removing this line will fix some of the discussed issues it also cleans up bad codestyle. So imho there is nothing against this pr.

@ikkez ikkez added the v4 label Aug 29, 2019
ikkez added a commit to ikkez/fatfree-core that referenced this pull request Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants