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

ModelHandler - Sort class names by psr-4 namespace sorting. #17

Closed
wants to merge 1 commit into from
Closed

ModelHandler - Sort class names by psr-4 namespace sorting. #17

wants to merge 1 commit into from

Conversation

sfadschm
Copy link
Contributor

@sfadschm sfadschm commented Nov 13, 2020

This draft aims to make the loading of models during the draft() process more constistent and might also be a small start for #7 .

Currently, to merge model names into the schema table, the ModelHandler follows this routine:

  1. Get all defined namespaces.
  2. Load (require) all file containing 'Models' within all namespaces.
  3. Get all defined model classes by get_declared_classes.
  4. Try to instantiate each model class and store the assigned table name.

This drawback here is, that get_declared_classes returns the class names ordered by their time of instantiation.
Therefore, depending on where the draft() method is called in the app, Some models will already have loaded (e.g., the 'Myth\Auth\Models\UserModel' when using myth-auth) and the resulting $classes array is unordered.

While this is not a problem as long as every table is only assigned to a single model, in the case of multiple models this can lead to inconsistent schema generation, as depending on the models 'pre-loaded' before the draft, different replacements are done (right now, always the last iterated model is stored in the schema table).

This addition sorts the $classes array by the order of the namespaces returned from the autoloader and hence by the order of the $psr4 config array. #7 states that multiple models are not supported yet, however, I think this addition might still be valuable in the current state, as behavior is now consistent with the user-defined namespace sorting in the $psr4 array.
When #7 is implemented in the future, I would think a similar logic will have to be applied to allow for priority ordering.

@MGatner
Copy link
Collaborator

MGatner commented Nov 14, 2020

I could see this being helpful, but I also want to leverage the framework's new Factories to assist with model discovery and caching: https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/concepts/factories.rst

By instantiating the model via Factories we will guarantee we are prioritizing App and increasing our chances of storing or reusing an instance.

@sfadschm
Copy link
Contributor Author

Just read into CI4 Factories, this indeed is a really useful tool and will likely be a much better replacement for the current model gathering. Looking forward to that :)

Closing, as this is rather a hacky workaround right now and will not be needed after Schemas next revision.

@sfadschm sfadschm closed this Nov 15, 2020
@sfadschm sfadschm reopened this Nov 15, 2020
@sfadschm
Copy link
Contributor Author

Okay, so I will have to sleep over this for one or two days...
New changes make sure that only those classes are grabbed that were previously targeted when loading file from the namespaces.

Added a note to the readme to explain model drafting behavior a little.

@sfadschm
Copy link
Contributor Author

Note: This way, if multiple models exists for a table, namespaces appearing later in the psr4 will overwrite those at the beginning (e.g. models in the App namespace are almost always overwritten).

@MGatner MGatner self-assigned this Dec 25, 2020
@sfadschm sfadschm closed this Apr 14, 2021
@sfadschm
Copy link
Contributor Author

That rebase went terrifyingly wrong...

@sfadschm sfadschm reopened this Apr 14, 2021
@sfadschm
Copy link
Contributor Author

sfadschm commented Jun 9, 2021

Just an additional notice on the topic:
Using PSR-4 is not a good solution for all cases.
E.g., I just switched to basing my CI4 on composer and composer's psr4 seems to be ordered simply by reversed alphabet, such that app will always come last.
however, this can be circumvented by defining composer loaded packages in the CI4 psr4 array (which kind of counteracts the goals of composer 😆 ).

@sfadschm
Copy link
Contributor Author

I think this can be closed for now.
Handling multiple models for a single table will be a larger refactor at a later time and the easy workaround for my specific case (having two models for a single table and wanting the App model to have precedence) can easily be achieved by adding the competing module namespace (here: Myth\Auth) to the $ignoredNamespaces config.
Don't know how I oversaw this all the time...

@MGatner
Copy link
Collaborator

MGatner commented Jul 10, 2021

Sounds good! Thanks for the feedback.

@MGatner MGatner closed this Jul 10, 2021
@sfadschm sfadschm deleted the order-models-by-psr4 branch July 22, 2021 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants