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

refactor: system/bootstrap.php only loads files and registers autoloader #5972

Merged
merged 11 commits into from
May 17, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented May 7, 2022

Description
When we want to use Preloading, we need CI4 Autoloader.
But it is not easy to make the Autoloader work fine.
If you use this system/bootstrap.php, you can make it a bit easily.

  • move CodeIgniter instantiation and DotEnv loading from system/bootstrap.php
  • tweak index.php and spark.php

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • [] Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the refactor Pull requests that refactor code label May 7, 2022
@kenjis kenjis marked this pull request as draft May 7, 2022 03:50
@kenjis kenjis changed the title refactor: move CodeIgniter instantiation from system/bootstrap.php refactor: system/bootstrap.php does only load files and autoloader registeration May 7, 2022
@kenjis kenjis changed the title refactor: system/bootstrap.php does only load files and autoloader registeration refactor: system/bootstrap.php does only load files and autoloader registeration May 7, 2022
@kenjis kenjis changed the title refactor: system/bootstrap.php does only load files and autoloader registeration refactor: system/bootstrap.php only loads files and registers autoloader May 7, 2022
@kenjis kenjis force-pushed the refactor-bootstrap branch 2 times, most recently from 46e4f8f to 5cba764 Compare May 7, 2022 04:56
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have to look at this o desktop, it's a little hard on mobile to tell what moved where.

I'm not sure the answer to this but... is changing the return of a file (bootstrap.php) considered a breaking change?

@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label May 7, 2022
@kenjis
Copy link
Member Author

kenjis commented May 7, 2022

I'm not sure the answer to this but... is changing the return of a file (bootstrap.php) considered a breaking change?

Yes. I added the changelog and upgrade guide.

@kenjis kenjis marked this pull request as ready for review May 7, 2022 23:45
@kenjis kenjis mentioned this pull request May 8, 2022
4 tasks
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand all the moves now. I don't think the bootstrap.php return is an issue - I'm actually glad we won't be relying on that anymore.

One phrasing suggestion, and let's make sure @lonnieezell has seen this one.

user_guide_src/source/installation/upgrade_420.rst Outdated Show resolved Hide resolved
@kenjis kenjis requested a review from lonnieezell May 10, 2022 12:53
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it already had a couple of other approvals I didn't scan every line. I have no problems with the changes, but I do think we should let people know why the changes are happening. My first thought on reading the title of the issue was, "Why are we changing? Is there any benefit to possibly breaking people's apps for this or is it just to be "proper"?" Turns out there is a very good reason, but let's help people get to that conclusion before they get upset lol.

@kenjis kenjis merged commit 8ddc566 into codeigniter4:develop May 17, 2022
@kenjis kenjis deleted the refactor-bootstrap branch May 17, 2022 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants