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

Allow loading Common.php function overrides #2101

Closed
lonnieezell opened this issue Jul 22, 2019 · 7 comments
Closed

Allow loading Common.php function overrides #2101

lonnieezell opened this issue Jul 22, 2019 · 7 comments
Labels
help wanted More help is needed for the proper resolution of an issue or pull request

Comments

@lonnieezell
Copy link
Member

Originally discussed in this thread: https://forum.codeigniter.com/thread-74096.html

There is currently no elegant way for a developer to override any of the methods in Common.php. I think this can be remedied very simply:

  • In system/bootstrap check for the existence of a file, app/Config/Functions.php (or maybe app/Functions.php) prior to loading the Common.php file.
  • If it exists load it in.

That's it. If a developer needs to override the function he can do it directly in that file or load other files that contain it, and not feel dirty editing public/index.php.

@lonnieezell lonnieezell added the help wanted More help is needed for the proper resolution of an issue or pull request label Jul 22, 2019
@lonnieezell lonnieezell added this to the 4.0.0-rc.1 milestone Jul 22, 2019
@jason-napolitano
Copy link
Contributor

jason-napolitano commented Jul 23, 2019

How about shipping Codeigniter >= RC1 with a blank/empty app/Common.php? This keeps the nomenclature consistent across the app/ and system/ paths and allows developers to replace core functions with their own, paired with the ability to create procedural functions that may be used anywhere in their application's codebase. One way to look at it is in terms of a "master helper" file.

Basically, if the function exists in app/Common.php use that function, and not the function inside of system/Common.php. This could be very simply checked with an if() during the bootstrap process.

I may be able to find time in my busy day to produce a PR for this too, since I'd love to contribute to the framework I've used for eons.

For example here's a simple solution without a PR. Consider it a proof of concept:

Replace:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/bootstrap.php#L99

require_once SYSTEMPATH  .  'Common.php';

With

if ( file_exists(APPPATH . 'Common.php) ) {
    require_once APPPATH . 'Common.php';
}

require_once SYSTEMPATH . 'Common.php';

This way, if app/Common.php exists, those functions will get loaded first, then system/Common.php. Forcing the if ( ! function_exists('...') ) checks in system/Common.php to fail, thus only loading functions from app/Common.php.

Sorry if this is a bit of a ramble, it is late and I am tired ha-ha. If you'd like a PR, let me know =D

@lonnieezell
Copy link
Member Author

I think that's the perfect way to handle it. It matches up with the blank BaseController we're shipping with to encourage it's use for adding common elements across all controllers, etc.

If you've got time for a PR that would be awesome. Otherwise someone will get to it before RC1 ships (hopefully very soon).

@jason-napolitano
Copy link
Contributor

jason-napolitano commented Jul 23, 2019

Okay cool beans. If I've time after picking up my car from the shop tomorrow after work, I will most certainly submit a PR. Maybe not with tests, but logic and docs would be my goal.

Thanks Lonnie and my apologies if I do not get to this by tomorrow evening.

@MGatner
Copy link
Member

MGatner commented Jul 23, 2019

I could also send this over. @jason-napolitano let me know if you aren't going to get to it.

@jason-napolitano
Copy link
Contributor

If you are faster than I am @MGatner then that'd be amazing. Just in case I don't get to it in a timely fashion =)

@jason-napolitano
Copy link
Contributor

jason-napolitano commented Jul 25, 2019

Pull request submitted. Sorry there are no docs for you. I am not at all familiar with the documentation library to build them in a timely fashion. However the logic and app/Common.php file is there. Hope it helps and please inform me of any further actions required by me, changes, etc.

EDIT: My apologies as well for the Travic-CI break. As long as I've been programming I've only issued one PR, and this is it and I am not too familiar with any continuous integration and the like (I posses an old-school, "save files my HDD as backups" mentality) =P.

@jim-parry
Copy link
Contributor

Fixed by #2105 and #2162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted More help is needed for the proper resolution of an issue or pull request
Projects
None yet
Development

No branches or pull requests

4 participants