Entity - Move default cast handlers to config

[najdanovicivan]

Description
The main reason for moving this to config is to be able to use Registars to extend the defaultCastHandles without having to modify every entity object.

Checklist:

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

[iRedds]

more configs for the god of configs

private -> protected && MyBaseEnity extends Entity

[kenjis]

It seems @iRedds 's way is enough.
Is it something missing?

[samsonasik]

This may require changelog update and upgrade_420.rst

https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.2.0.rst
https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/installation/upgrade_420.rst

btw, @kenjis for mandatory file changes note for public/index.php and spark:

https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/installation/upgrade_420.rst#id7

it seems it needs to be in changelog as well.

[MGatner]

@najdanovicivan specifically mentions Registrars. While using a BaseEntity would solve this for an app it does not allow things like libraries to offer their own cast extensions.

I know we've piled a lot into Config but it is our pivot point for extendability. I think one way to help this will be to move all files that are actually BaseConfigs into their own system namespace and then only have a few in App that extend them - this way any given app's config folder might only have 4-5 files instead of 25.

[najdanovicivan]

@najdanovicivan specifically mentions Registrars. While using a BaseEntity would solve this for an app it does not allow things like libraries to offer their own cast extensions.

I know we've piled a lot into Config but it is our pivot point for extendability. I think one way to help this will be to move all files that are actually BaseConfigs into their own system namespace and then only have a few in App that extend them - this way any given app's config folder might only have 4-5 files instead of 25.

I was speaking about same stuff a while ago. It will be best if we can have all the default configs under CodeIgniter/Config namespace so that Config namespace contains only overrides. It will make updates much more easier in the future since there won't be missing parameters when new configs are introduced.

[kenjis]

@samsonasik

it seems it needs to be in changelog as well.

I sent a PR #5837

[kenjis]

I think one way to help this will be to move all files that are actually BaseConfigs into their own system namespace and then only have a few in App that extend them - this way any given app's config folder might only have 4-5 files instead of 25.

I agree that. I feel there are too many config files in app/Config/.

[lonnieezell]

Is it a better experience for the developer to have 4 config files and then have to hunt down system config files, copy them to the app config folder and modify from there? To me it seems the cleanliness of the config folder like that would makes things more of a pain for the end user.

As framework developers we can either be very opinionated, and not need as much configuration options, or be very flexible and give the developer the opportunity to change the system to be what they need it to be. CodeIgniter has always been more of the latter type of framework. One reason we have more config files now is because we split the app config file into separate files to makes things more clear and easy to find.

I'm all for having original copies in the system folder. This allows us to add config settings and not break apps that miss adding new options to their existing config files.

[MGatner]

I think we could do a better job of splitting out "basic" from "advanced" config. For example, I would expect someone making their own Exception or View extensions to know what they were doing enough already to migrate a Config from system to app, whereas just about everyone needs to open up Routes.php in every app.

It also isn't just about the files - if all of our App versions extend their parent classes then we can keep their content slimmer and leave out "advanced" options. For example Cache.php could be trimmed down to four properties instead of the current 11, many of which won't ever be touched.

If this is the direction we plan to head then I would be in favor of moving this new Entity Config to system/Config/ and not including it in App at all, as I think it is highly unlikely for the general developer to need their own cast handler. The caveat to this is we will need clear docs on how to extend them, preferably with an example "walkthrough".

[lonnieezell]

What is the benefit for the developer of hiding configuration options? I personally don't see any - only the possibility of confusion. I know I personally would be irritated if I had to go snooping through system files to discover there was a configuration option. To me, having them in the app config file aids discoverability, causes less friction when it is needed, and doesn't cause any harm for people to ignore them. Kind of the php.ini :)

I suppose we do have a little precedent for this with Services, though that feels different to me.

[sfadschm]

I would think the benefit is less work when updating CodeIgniter, as the app folder has to be updated manually everytime. While there are tools to assist with that, it is still more work.

[najdanovicivan]

@lonnieezell regarding the configs we can still have all the files in Config namespace but instead of having

class App extends BaseConfig

we'll have

class App extends CodeIgniter\Config\App

That way there will be no need to hunt down for any files when it's needed to make changes to the configs. As override files will already be in place. It just gives us the ability for framework to work without config files as those will be loaded from system namespace. More importantly if there is new config class or parameter introduced it will be available in the system namespace so when framework is updated it will not crash due to missing configs.

[lonnieezell]

@najdanovicivan I agree, and believe I said earlier I'm ok with that. I just wasn't sure hiding config options in /app was a good idea.

[najdanovicivan]

What else I need to do for the time being so that this one can be merged ?

[MGatner]

Looking back over this @najdanovicivan I think we would still like to see a use case that requires this. Registrars are great but they allow modules to add functionality to App classes that can "discover" that functionality, and I don't see an App entity ever "discovering" a cast type it doesn't already need to know exists. Simply adding your new casts to MyModule\Entities\Widget::$castHandlers appears to have the desired effect already:

$handlers = array_merge($this->defaultCastHandlers, $this->castHandlers);

---

Also, my apologies that this PR got hijacked into a discussion of Config behavior which led to its neglect.

[kenjis]

It seems to me that overuse of Config and Registrar is not a good idea.
The idea of Entity Config does not feel right.

I still do not understand the need for this PR.
It seems sufficient to create (extend) your own Entity class and use it.

[MGatner]

@najdanovicivan If you wish to pursue this please address the questions above or start a forum thread to gauge community interest in this as a new feature. Closing as stale for now.