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

CI4: OSPOS Function signatures not compatible with CI4 #3602

Closed
objecttothis opened this issue Nov 28, 2022 · 3 comments
Closed

CI4: OSPOS Function signatures not compatible with CI4 #3602

objecttothis opened this issue Nov 28, 2022 · 3 comments

Comments

@objecttothis
Copy link
Member

With CI4 comes a number of built in CRUD functions by the same name as some of our model functions. The problem is that the function signatures do not match, so it throws a runtime exception.
image

This can be resolved one of two ways

  1. Rename functions so they are no longer inheriting their base function. This involves refactoring but potentially less in terms of code rewrite.
  2. Keep function names but rework signatures and functions to match their base functions.
  3. A hybrid of 1 and 2. Rework the low hanging fruit (easy ones), rename the more complicated ones and create a technical debt to go back and rework the renamed functions after the CI4 upgrade has been merged into the master.

No one likes code that looks like a rat's nest, so I prefer 2, but unless more people are able to contribute to this upgrade, we may need to compromise by doing 3. @jekkos @daN4cat @opensourcepos/org thoughts?

closed by #3592

@objecttothis objecttothis added this to the 3.4.0 milestone Nov 28, 2022
@objecttothis objecttothis self-assigned this Nov 28, 2022
@objecttothis objecttothis linked a pull request Nov 28, 2022 that will close this issue
34 tasks
@jekkos
Copy link
Member

jekkos commented Nov 29, 2022

What does the base function exactly do? If its internal kitchen then we perhaps better change the signatures. I agree that we should strive to make changes consistent.

@objecttothis
Copy link
Member Author

https://codeigniter.com/user_guide/models/model.html#deleting-data

They are cookie-cutter CRUD functions meant for use in the application. In many cases I think we may be unnecessarily overriding them because CI4 provides the functionality we want. For example we can do $attribute->where('attribute_id', $attribute_id)->delete(); and no need to override. The only caveat with delete specifically is that we want soft deletes. The way CI_4 does this is with a $useSoftDeletes boolean in the model set to true and a datetime field in the table called deleted_at then the base function will populate the field with the current datetime. I think this is a more elegant solution than our current deleted flag because it not only tells you that it is soft deleted but it tells you when this was done. You can still hard delete the value by adding true into the 2nd parameter of the delete function. That said, perhaps we consider refactoring and database changes associated as a separate changeset and just simply override the functions or rename then set a technical debt to implement CI4 style delete in the code. I'm pretty sure the create, read and update functions don't need much in the way of refactoring like delete would.

@objecttothis objecttothis removed their assignment Dec 1, 2022
@objecttothis objecttothis changed the title OSPOS Function signatures not compatible with CI4 CI4: OSPOS Function signatures not compatible with CI4 Dec 9, 2022
@objecttothis
Copy link
Member Author

I believe I resolved all of these. Will reopen if I find more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants