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

feat: add Factories::define() to explicitly override a class #7733

Merged
merged 27 commits into from
Jul 29, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jul 24, 2023

Description
Fixes #7694

  • add Factories::define() to explicitly override a class
  • [BC] preferApp works only for no namespaced classname
  • [BC] rename Factories::$basenames to $aliases

I would like to deprecate preferApp option.

Changes
Property structure:

$basenames = [component => [basename => FQCN]];
↓
$aliases = [component => [alias => FQCN]];

basename is a short classname, or short classname with sub-directories.
alias is a FQCN, short classname, or short classname with sub-directories.
Factories::define() sets $aliases.

Behavior:

  • model(\Myth\Auth\Models\UserModel::class) or model('Myth\Auth\Models\UserModel')
    • before:
      • returns App\Models\UserModel if exists and preferApp is true
      • returns Myth\Auth\Models\UserModel if exists and preferApp is false
    • after:
      • returns Myth\Auth\Models\UserModel even if preferApp is true
      • returns App\Models\UserModel if you define Factories::define('models', 'Myth\Auth\Models\UserModel', 'App\Models\UserModel') before the first call of the model()

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 bug Verified issues on the current code behavior or pull requests that will fix them enhancement PRs that improve existing functionalities breaking change Pull requests that may break existing functionalities 4.4 labels Jul 24, 2023
@kenjis kenjis force-pushed the feat-Factories-define branch 2 times, most recently from 1294ee7 to 4716098 Compare July 24, 2023 07:36
@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Jul 24, 2023
@kenjis kenjis marked this pull request as draft July 24, 2023 09:38
@kenjis kenjis marked this pull request as ready for review July 24, 2023 10:52
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.

Can you give an example of how this would affect an existing workflow? For example right now Myth:Auth controller might use:

$user = model('UserModel')->find($userId);

... and a developer would supply app/Models/UserModel.php to override. With this update, would the developer need to add something to boot like:

use App\Models\UserModel;

Factories::define('models', 'UserModel', UserModel::class);

?

Comment on lines 82 to 83
* @param string $name Classname. The first parameter of Factories magic method
* @param string $classname FQCN to load
Copy link
Member

Choose a reason for hiding this comment

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

These can get mixed up very easily. Let's stick to something like "shortname" for the unqualified version.

Suggested change
* @param string $name Classname. The first parameter of Factories magic method
* @param string $classname FQCN to load
* @param string $name Class shortname. The first parameter of Factories magic method
* @param string $classname FQCN to load

Copy link
Member

Choose a reason for hiding this comment

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

Or I see I called it "basename" below. Doesn't matter as long as we are consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I introduced a new term "class alias".

system/Config/Factories.php Show resolved Hide resolved
if ($options['preferApp'] && class_exists($appname) && self::verifyInstanceOf($options, $name)) {
if (
// preferApp is used only for no namespace class.
strpos($name, '\\') === false
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only breaking change? It seems like the rest of this PR could be rolled out separately without as big an impact.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps.

And it is what I want to change. Also it is Paul's opinion. See #7694 (comment)

system/Config/Factories.php Show resolved Hide resolved
@kenjis
Copy link
Member Author

kenjis commented Jul 25, 2023

@MGatner No. model('UserModel') specifies no namespaced classname 'UserModel'. So Factories look for App\Models\UserModel (preferApp works).

But if there is code like model(\Myth\Auth\Models\UserModel::class) or model('Myth\Auth\Models\UserModel'), Factories returns Myth\Auth\Models\UserModel if you don't define anything.

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.

Nice work!

kenjis added 14 commits July 26, 2023 09:00
Except for Config, if FQCN is specified, preferApp is ignored and that class is loaded.
…instance as `Config\Validation`

When you call `Factories::injectMock('config', 'Validation', $config)`,
but if the production code calls `config(\Config\Validation::class)`,
the mock instance will not be used and fail the test:
CodeIgniter\Database\ModelFactoryTest::testBasenameReturnsExistingNamespaceInstance
@kenjis kenjis marked this pull request as draft July 26, 2023 00:35
@kenjis kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Jul 26, 2023
@kenjis
Copy link
Member Author

kenjis commented Jul 26, 2023

Rebased and added commits from a3c768a test: add test.

@kenjis kenjis merged commit 5695e7a into codeigniter4:4.4 Jul 29, 2023
@kenjis kenjis deleted the feat-Factories-define branch July 29, 2023 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants