trongate / trongate-framework

The Trongate PHP framework
https://trongate.io
Other
1.11k stars 100 forks source link

update_custom an addition function for the database model #151

Closed monxian closed 10 months ago

monxian commented 1 year ago

This is a function that can use any field for the update instead of just the table id. It's just a slight modification of the update function Function arguments are (field value, $data(the data you want to change),field name, table name)

trongate commented 1 year ago

Hello!

Awesome AWESOME AWESOME idea! Thank you!

However, I think your implementation could be even better. You have:

function update_custom($update_value, $data, $field, $target_tbl = null){

I think you should have:

public function update($update_id, $data, $target_tbl = null, $column=null)

Here's a breakdown of why I'm suggesting this:

*.By flipping the order of the arguments, it means that the 'custom' update will be an optional and non-breaking change.

*.I also think that changing the variable name to 'column' makes the syntax more consistent with the rest of the model file, for example 'get_where_custom()' uses column.

CONS

The downside of what I'm suggesting is that users will have to explicitly declare the table name when calling the query. However, I think people will be okay with that.

WHAT DO YOU THINK?

Do you agree with my suggestions? If you do then I'd be grateful if you could carry out those tweaks and make them part of a pull request. I am - of course - delighted to give you full credit.

Many thanks for your help. I cannot stress enough how grateful I and everyone else is for having people like you on board. Thank you!

DC

monxian commented 1 year ago

Yes, I can do another.

On Fri, Sep 1, 2023, 5:51 AM Trongate @.***> wrote:

Hello!

Awesome AWESOME AWESOME idea! Thank you!

However, I think your implementation could be even better. You have:

function update_custom($update_value, $data, $field, $target_tbl = null){

I think you should have:

public function update($update_id, $data, $target_tbl = null, $column=null)

Here's a breakdown of why I'm suggesting this:

  • I think it's an important enough change to warrant going onto the main 'update' function. I don't think we should introduce a new one.

*.By flipping the order of the arguments, it means that the 'custom' update will be an optional and non-breaking change.

*.I also think that changing the variable name to 'column' makes the syntax more consistent with the rest of the model file, for example 'get_where_custom()' uses column.

CONS

The downside of what I'm suggesting is that users will have to explicitly declare the table name when calling the query. However, I think people will be okay with that.

WHAT DO YOU THINK?

Do you agree with my suggestions? If you do then I'd be grateful if you could carry out those tweaks and make them part of a pull request. I am - of course - delighted to give you full credit.

Many thanks for your help. I cannot stress enough how grateful I and everyone else is for having people like you on board. Thank you!

DC

— Reply to this email directly, view it on GitHub https://github.com/trongate/trongate-framework/pull/151#issuecomment-1702484157, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVLIODDOYDSNQA6G22GHCTXYGV25ANCNFSM6AAAAAA32Z7ZNM . You are receiving this because you authored the thread.Message ID: @.***>

trongate commented 10 months ago

I'm going to knock this one into the long grass - for the moment - but only because I don't entirely understand it. I'd very much be a more in depth conversation about your suggestions. You're awesome and I hope we can catch up soon.