tattersoftware / codeigniter4-schemas

Database schema management, for CodeIgniter 4
MIT License
22 stars 10 forks source link

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

Closed sfadschm closed 3 years ago

sfadschm commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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).

sfadschm commented 3 years ago

That rebase went terrifyingly wrong...

sfadschm commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Sounds good! Thanks for the feedback.