tattersoftware / codeigniter4-relations

Entity relationships for CodeIgniter 4
MIT License
87 stars 20 forks source link

Being able to load a schema manually #40

Closed daycry closed 2 years ago

daycry commented 2 years ago
MGatner commented 2 years ago

The Schemas library can already take a manually-loaded Schema, which would in turn be used by Relations: https://github.com/tattersoftware/codeigniter4-schemas/blob/cee97dbe49fe5d7ce57b756bc1f61500017db13d/src/Schemas.php#L85

I would need to hear a strong supporting case on why an individual Model/Entity should use its own manually-provided Schema before approving this.

daycry commented 2 years ago

But the vendor relations, collects the data by querying the database, and does not allow the model to be loaded manually to avoid calls to the database.

With the change I propose you can load the model, for example, through directory instead of using the $chemas->get() function, used in the Base trait.

On Tue, 26 Jul 2022, 15:48 MGatner, @.***> wrote:

The Schemas library can already take a manually-loaded Schema, which would in turn be used by Relations: https://github.com/tattersoftware/codeigniter4-schemas/blob/cee97dbe49fe5d7ce57b756bc1f61500017db13d/src/Schemas.php#L85

I would need to hear a strong supporting case on why an individual Model/Entity should use its own manually-provided Schema before approving this.

— Reply to this email directly, view it on GitHub https://github.com/tattersoftware/codeigniter4-relations/pull/40#issuecomment-1195509561, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZ5DPY27NGCU6SY2BYLEM3VV7UDXANCNFSM54V7UUBA . You are receiving this because you authored the thread.Message ID: @.***>

MGatner commented 2 years ago

That all makes sense, but you can still accomplish the same thing using Schemas. If you call service('schemas')->setSchema($schema) with the same input that you would pass into your BaseEntity::setSchema() it will use that schema instead of loading from the database. I'm trying to find out how someone would use your new method instead did the existing one. Introducing a "local schema" for a model or entity opens up a can of worms and could be very confusing.

daycry commented 2 years ago

For example, if I use a UserModel() With relations, when you call a find() funtion It call $schemas->get(), and every model that use relations trait Will call the same function another time. If we can call setSchema like this: $schema= (new DirectoryHandler())->draft(); $userModel->setSchema($schema)->with(keys)->find(); you can avoid call schema->get() on the fly.

I think that is an interesting feature, for my proyects is usefull when I work with a lot of databases in the same proyect.

On Tue, 26 Jul 2022, 20:23 MGatner, @.***> wrote:

That all makes sense, but you can still accomplish the same thing using Schemas. If you call service('schemas')->setSchema($schema) with the same input that you would pass into your BaseEntity::setSchema() it will use that schema instead of loading from the database. I'm trying to find out how someone would use your new method instead did the existing one. Introducing a "local schema" for a model or entity opens up a can of worms and could be very confusing.

— Reply to this email directly, view it on GitHub https://github.com/tattersoftware/codeigniter4-relations/pull/40#issuecomment-1195830814, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZ5DP5D5OUBX7VUBMK6VNLVWAUK3ANCNFSM54V7UUBA . You are receiving this because you authored the thread.Message ID: @.***>

MGatner commented 2 years ago

So is the issue that you have multiple databases and you need to be able to enforce a specific database's schema on a model?

I don't think calling $schemas->get() is really a performance concern. After the schema is loaded initially this will just be a simple property fetch from the library class. By "injecting" local schemas you're gaining a tiny performance boost in exchange for the extra memory, which can grow rather large depending on how complex your schema is.

daycry commented 2 years ago

Yes, exacly.

This is the issue, directoryHandler is an example I prefer use DatabaseHandler and save It on redis.

But relations can't work with differents databases I proposed this feature, because I can't said the group of database that I want work in the relations library.

On Tue, 26 Jul 2022, 20:42 MGatner, @.***> wrote:

So is the issue that you have multiple databases and you need to be able to enforce a specific database's schema on a model?

I don't think calling $schemas->get() is really a performance concern. After the schema is loaded initially this will just be a simple property fetch from the library class. By "injecting" local schemas you're gaining a tiny performance boost in exchange for the extra memory, which can grow rather large depending on how complex your schema is.

— Reply to this email directly, view it on GitHub https://github.com/tattersoftware/codeigniter4-relations/pull/40#issuecomment-1195850955, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZ5DP5TTX6JTUEH2B2NQADVWAWSRANCNFSM54V7UUBA . You are receiving this because you authored the thread.Message ID: @.***>

daycry commented 2 years ago

For example:

[image: image.png]

In this example I get the schema with schemas library and I set the schema variable in relation library.

It works with my changes.

El mar, 26 jul 2022 a las 20:54, Jordi daycry @.***>) escribió:

Yes, exacly.

This is the issue, directoryHandler is an example I prefer use DatabaseHandler and save It on redis.

But relations can't work with differents databases I proposed this feature, because I can't said the group of database that I want work in the relations library.

On Tue, 26 Jul 2022, 20:42 MGatner, @.***> wrote:

So is the issue that you have multiple databases and you need to be able to enforce a specific database's schema on a model?

I don't think calling $schemas->get() is really a performance concern. After the schema is loaded initially this will just be a simple property fetch from the library class. By "injecting" local schemas you're gaining a tiny performance boost in exchange for the extra memory, which can grow rather large depending on how complex your schema is.

— Reply to this email directly, view it on GitHub https://github.com/tattersoftware/codeigniter4-relations/pull/40#issuecomment-1195850955, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZ5DP5TTX6JTUEH2B2NQADVWAWSRANCNFSM54V7UUBA . You are receiving this because you authored the thread.Message ID: @.***>

MGatner commented 2 years ago

Your image didn't come through.

I am beginning to see your issue, but it still doesn't make me think this is a good change for the library. You could implement this yourself on a BaseModel and use it throughout your project. I don't want to promote a solution that creates multiple instance-specific Schemas; I think the official fix for this is to improve multiple-database handling in Tatter\Schemas.

daycry commented 2 years ago

Ok,

Thank you for tour attention. I Will do a fork with my changes as a workarround, but I Will think how I can help us with multi-databases issue when I finish my active projects.

Thank you so much.

On Wed, 27 Jul 2022, 13:37 MGatner, @.***> wrote:

Your image didn't come through.

I am beginning to see your issue, but it still doesn't make me think this is a good change for the library. You could implement this yourself on a BaseModel and use it throughout your project. I don't want to promote a solution that creates multiple instance-specific Schemas; I think the official fix for this is to improve multiple-database handling in Tatter\Schemas.

— Reply to this email directly, view it on GitHub https://github.com/tattersoftware/codeigniter4-relations/pull/40#issuecomment-1196614657, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZ5DP5QC3YLDVV2NTVHZZ3VWENPNANCNFSM54V7UUBA . You are receiving this because you authored the thread.Message ID: @.***>