- Notifications
You must be signed in to change notification settings - Fork2k
feat: ImproveRegistrars#9445
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:4.7
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f70d2d8 to5421889CompareSorry, 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. |
New features won't be accepted in the develop branch. |
This does not apply to PR - a simple reminder. In general, can the argument with |
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 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. |
No. It is not necessary to use If you're worried about problems, then what do you say that anyone can replace with any value instead of the original type 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 onapp/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? |
Uh oh!
There was an error while loading.Please reload this page.
5421889 tob3e6c5bComparebf6b741 toff79dcbCompareI moved to the next branch It's worth merging thedevelop branch into4.7 to continue development. There are errors in phpstan and rector. |
I have updated4.7 to latest develop. |
I have a thought - will adding an event after applying theRegistrar help? publicfunction__construct() {static::$moduleConfig ??=newModules();if (!static::$override) {return; }$this->registerProperties(); Events::trigger('post_config');// .... } I haven't tested it, maybe |
👋 Hi,@neznaika0! |
ff79dcb toc2eb405Comparec2eb405 tof29a8c0Compare👋 Hi,@neznaika0! |
Uh oh!
There was an error while loading.Please reload this page.
Description
See#9231
RegistrarModulesNow 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.
Thedevelop 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 theA-Z order can be overwritten.
Checklist: