CodeIgniter4 icon indicating copy to clipboard operation
CodeIgniter4 copied to clipboard

feat: add EntityInterface

Open colethorsen opened this issue 2 years ago • 17 comments

The main CodeIgniter entity couldn't be replaced with a custom one because the database drivers checked to see if entities were extensions of the entity itself and not an interface. Creating an interface allows for a completely new entity to implement the interface instead of having to extend the Entity itself.

Checklist:

  • [x] Securely signed commits
  • [ ] Component(s) with PHPDoc blocks, only if necessary or adds value
  • [ ] Unit testing, with >80% coverage
  • [ ] User guide updated
  • [x] Conforms to style guide

colethorsen avatar Dec 12 '23 18:12 colethorsen

I'm all for this. I am not sure if all of the methods in the interface need to be in the interface, though, and will have to look over that later.

lonnieezell avatar Dec 12 '23 19:12 lonnieezell

They probably don't from a methods utilized within the framework itself standpoint, we'd need to identify which ones are being used though to include. This is just all the public methods which seemed like the safest place to start.

colethorsen avatar Dec 12 '23 20:12 colethorsen

In this case, the interface needs only the injectRawData() method. If you need other methods, it is better to add other interfaces. See https://en.wikipedia.org/wiki/Interface_segregation_principle

kenjis avatar Dec 12 '23 21:12 kenjis

Is this a bug fix? If not, please go to 4.5 branch.

kenjis avatar Dec 12 '23 21:12 kenjis

In this case, the interface needs only the injectRawData() method. If you need other methods, it is better to add other interfaces.

I think given that this is to align with how you'd build a replacement entity, you'd at minimum want to require the implementation of those methods used in the model such as toRawArray()

colethorsen avatar Dec 12 '23 22:12 colethorsen

Here's my list of methods we would want in the interface:

  • fill - it's too much a part of how I use an entity to imagine it not being there. Especially dealing with data from forms.
  • toArray and toRawArray
  • hasChanged
  • injectRawData since that's how the database injects data.

To me, those 5 methods are what defines the core of the functionality if you were to create your own.

lonnieezell avatar Dec 13 '23 04:12 lonnieezell

Indeed, injectRawData() and toRawArray() can be considered a set.

But there is an opinion that hasChanged() should be removed. It is not a must. It is better to add as another interface, if we create interface for it.

kenjis avatar Dec 13 '23 06:12 kenjis

I would be in favor of adding bare minimum methods to the interface, which will ensure that everything will work properly with our model. While we may be used to more methods, I feel the custom Entity implementation should not impose anything beyond what is required.

That being said I agree with @kenjis - we need only injectRawData() and toRawArray() since no other methods are required elsewhere.

This option would also require a little mention in the user guide.

michalsn avatar Dec 13 '23 09:12 michalsn

If thinking of it purely as a "what does the framework need" to work with an Entity than I agree with both of you. I was thinking of it more from a user's perspective of what's the bare minimum functionality I would need to be able to reasonably swap out usage of Entity types.

lonnieezell avatar Dec 13 '23 14:12 lonnieezell

With the point of this interface being to allow for the safe creation of a custom entity, and assuming that the only items required by the framework are injectRawData() and toRawArray() it probably makes sense to just include those. The others are all convenience and really the point of being able to create your own entity is that you might want to handle data differently or create a much more minimal entity and therefore the others probably shouldn't be required.

Even hasChanged() while likely something that would be pretty common is not required by the other 2 toRawArray() alreaady takes care of the change state through arguments.

colethorsen avatar Dec 13 '23 16:12 colethorsen

@colethorsen - out of curiosity, why are you implementing your own Entity class? Are you missing some functionality or just have a custom use case?

michalsn avatar Dec 13 '23 16:12 michalsn

Thank you for updating.

I think this is an enhancement, not a bug fix. So please change the base branch to 4.5. https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#if-you-sent-to-the-wrong-branch

kenjis avatar Dec 14 '23 23:12 kenjis

@colethorsen - out of curiosity, why are you implementing your own Entity class? Are you missing some functionality or just have a custom use case?

Cole is one of our resident NoSQL devs. In my work with Firebase I definitely had to hack up Entity to get things to work, and eventually ditched it altogether - so my guess is that he's doing the same.

MGatner avatar Dec 28 '23 11:12 MGatner

Please fix the code, and re-run composer cs-fix.

1 file with changes
===================

1) system/Entity/EntityInterface.php:0

    ---------- begin diff ----------
@@ @@
 <?php

+declare(strict_types=1);
+
 /**
  * This file is part of CodeIgniter 4 framework.
  *
@@ @@
  * For the full copyright and license information, please view
  * the LICENSE file that was distributed with this source code.
  */
-
 namespace CodeIgniter\Entity;

 /**
    ----------- end diff -----------

kenjis avatar Jan 28 '24 11:01 kenjis

Thank you. Can you add a short description to the changelog? https://github.com/codeigniter4/CodeIgniter4/blob/4.5/user_guide_src/source/changelogs/v4.5.0.rst#enhancements

kenjis avatar Jan 30 '24 23:01 kenjis

@codeigniter4/core-team Is there no need to add test code?

kenjis avatar Jan 30 '24 23:01 kenjis

:wave: Hi, @colethorsen!

We detected conflicts in your PR against the base branch :speak_no_evil:
You may want to sync :arrows_counterclockwise: your branch with upstream!

Ref: Syncing Your Branch

github-actions[bot] avatar Feb 17 '24 03:02 github-actions[bot]

BaseResult still depends on Entity. Replace it with the interface. https://github.com/codeigniter4/CodeIgniter4/blob/b17255a43fbe94dd6fcaeebc93303e04eae32bc6/system/Database/BaseResult.php#L16 https://github.com/codeigniter4/CodeIgniter4/blob/b17255a43fbe94dd6fcaeebc93303e04eae32bc6/system/Database/BaseResult.php#L162 https://github.com/codeigniter4/CodeIgniter4/blob/b17255a43fbe94dd6fcaeebc93303e04eae32bc6/system/Database/BaseResult.php#L243 https://github.com/codeigniter4/CodeIgniter4/blob/b17255a43fbe94dd6fcaeebc93303e04eae32bc6/system/Database/BaseResult.php#L542

kenjis avatar Mar 22 '24 01:03 kenjis

Fix the PHPStan errors. Remove the following Ignored error pattern in phpstan-baseline.php.

Error: Ignored error pattern #^Method CodeIgniter\\Entity\\Entity\:\:injectRawData\(\) has parameter \$data with no value type specified in iterable type array\.$# in path /home/runner/work/CodeIgniter4/CodeIgniter4/system/Entity/Entity.php was not matched in reported errors.
Error: Ignored error pattern #^Method CodeIgniter\\Entity\\Entity\:\:toRawArray\(\) return type has no value type specified in iterable type array\.$# in path /home/runner/work/CodeIgniter4/CodeIgniter4/system/Entity/Entity.php was not matched in reported errors.
 ------ ----------------------------------------------------------------------- 
  Line   system/Entity/Entity.php                                               
 ------ ----------------------------------------------------------------------- 
         Ignored error pattern #^Method                                         
         CodeIgniter\\Entity\\Entity\:\:injectRawData\(\) has parameter \$data  
         with no value type specified in iterable type array\.$# in path        
         /home/runner/work/CodeIgniter4/CodeIgniter4/system/Entity/Entity.php   
         was not matched in reported errors.                                    
         Ignored error pattern #^Method                                         
         CodeIgniter\\Entity\\Entity\:\:toRawArray\(\) return type has no       
         value type specified in iterable type array\.$# in path                
         /home/runner/work/CodeIgniter4/CodeIgniter4/system/Entity/Entity.php   
         was not matched in reported errors.                                    
 ------ ----------------------------------------------------------------------- 


Error:  [ERROR] Found 2 errors  

https://github.com/codeigniter4/CodeIgniter4/actions/runs/8423534292/job/23065390399?pr=8325

kenjis avatar Mar 26 '24 01:03 kenjis

:wave: Hi, @colethorsen!

We detected conflicts in your PR against the base branch :speak_no_evil:
You may want to sync :arrows_counterclockwise: your branch with upstream!

Ref: Syncing Your Branch

github-actions[bot] avatar Mar 28 '24 03:03 github-actions[bot]

Resolve conflicts, please.

kenjis avatar Mar 28 '24 22:03 kenjis

We will release v4.5.0 soon. If you want this to be included, fix the issue.

kenjis avatar Apr 02 '24 23:04 kenjis