CodeIgniter4 icon indicating copy to clipboard operation
CodeIgniter4 copied to clipboard

feat: Improve `Registrars`

Open neznaika0 opened this issue 11 months ago • 10 comments

Description See #9231

  • Added getting the current config properties in Registrar
  • The behavior is configured in Modules

Now you can add, replace, and recursively replace arrays yourself. In Registrar (with the option enabled), apply any changes based on the input data. The name of the argument is not important as long as we trust the user.

This is a draft stage, if you approve it, I will supplement the documentation and corrections.

The develop branch because it doesn't break existing code until the option is enabled.

I noticed that there is no mention of the configuration rewriting order anywhere. The first modules in the A-Z order can be overwritten.

Checklist:

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

neznaika0 avatar Feb 07 '25 22:02 neznaika0

Sorry, but this doesn't make sense to me.

In the proposed implementation, the most recently loaded module wins and overwrites the values previously set by other modules.

michalsn avatar Feb 08 '25 18:02 michalsn

New features won't be accepted in the develop branch.

michalsn avatar Feb 08 '25 18:02 michalsn

This does not apply to PR - a simple reminder. In general, can the argument with $this be accepted?

neznaika0 avatar Feb 08 '25 20:02 neznaika0

Well, it would have been nice if you had mentioned that currently, only tests are working.

So the changes that will take place can be boiled down to the fact that instead of array_merge(), we will use array_replace_recursive()? Are you planning anything else?

IMO if you want to replace anything that is an array, you should rather modify the base config file and not try to use Registrars. What you’re planning should work for associative arrays, but will fail miserably for indexed arrays.

From my perspective, this function is not desirable - mainly because it will cause more problems than good. And more "shortcomings" will only multiply - because I can think of a few.

The solution would be to encapsulate this in some kind of manager, but... Registrars were invented with very simple operations in mind, and they should stay that way.

For my part, I have already expressed my opinion on the subject. If you find someone who supports this idea - go ahead.

michalsn avatar Feb 09 '25 07:02 michalsn

No. It is not necessary to use array_* inside. This gives the flexibility for the user to change the values as he wants.

If you're worried about problems, then what do you say that anyone can replace with any value instead of the original type string => int ? You can say it's a bug. Otherwise, flexibility. This does not negate the fact that the array can still be broken without this PR (example, in Filters ['before' => ...].

In addition, it is a configurable option.

If things are that bad, CI doesn't have a good way to work with configs. They all depend heavily on app/Config/*. The user always needs to manually add changes for each package. It seems that this is all it is designed for.

@paulbalandan @kenjis @MGatner @samsonasik Share your opinion?

neznaika0 avatar Feb 09 '25 07:02 neznaika0

I moved to the next branch

It's worth merging the develop branch into 4.7 to continue development. There are errors in phpstan and rector.

neznaika0 avatar Feb 23 '25 17:02 neznaika0

I have updated 4.7 to latest develop.

paulbalandan avatar Feb 23 '25 17:02 paulbalandan

I have a thought - will adding an event after applying the Registrar help? After the configuration is fully initialized, it can be changed before being used in other places.

    public function __construct()
    {
        static::$moduleConfig ??= new Modules();

        if (! static::$override) {
            return;
        }

        $this->registerProperties();
        Events::trigger('post_config');
        // ....
    }

I haven't tested it, maybe pre_system can be used too.

neznaika0 avatar Mar 01 '25 05:03 neznaika0

:wave: Hi, @neznaika0!

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 01 '25 14:03 github-actions[bot]

:wave: Hi, @neznaika0!

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 Apr 20 '25 06:04 github-actions[bot]

Unfortunately, we barely discuss common questions.. I'm closing the PR.

neznaika0 avatar Nov 06 '25 12:11 neznaika0