Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Draft
neznaika0 wants to merge3 commits intocodeigniter4:4.7
base:4.7
Choose a base branch
Loading
fromneznaika0:feat/improve-registrars

Conversation

@neznaika0
Copy link
Contributor

@neznaika0neznaika0 commentedFeb 7, 2025
edited
Loading

Description
See#9231

  • Added getting the current config properties inRegistrar
  • The behavior is configured inModules

Now you can add, replace, and recursively replace arrays yourself. InRegistrar (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:

  • 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

@michalsn
Copy link
Member

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
Copy link
Member

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

@neznaika0
Copy link
ContributorAuthor

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

@michalsn
Copy link
Member

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 ofarray_merge(), we will usearray_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.

@neznaika0
Copy link
ContributorAuthor

No. It is not necessary to usearray_* 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 typestring => 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 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?

@samsonasiksamsonasik added the wrong branchPRs sent to wrong branch labelFeb 15, 2025
@neznaika0neznaika0 changed the base branch fromdevelop to4.7February 23, 2025 14:42
@neznaika0neznaika0force-pushed thefeat/improve-registrars branch 3 times, most recently frombf6b741 toff79dcbCompareFebruary 23, 2025 16:50
@neznaika0
Copy link
ContributorAuthor

I moved to the next branch

It's worth merging thedevelop branch into4.7 to continue development. There are errors in phpstan and rector.

@paulbalandan
Copy link
Member

I have updated4.7 to latest develop.

@michalsnmichalsn removed the wrong branchPRs sent to wrong branch labelFeb 25, 2025
@neznaika0
Copy link
ContributorAuthor

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

publicfunction__construct()    {static::$moduleConfig ??=newModules();if (!static::$override) {return;        }$this->registerProperties();        Events::trigger('post_config');// ....    }

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

@github-actionsgithub-actionsbot added the stalePull requests with conflicts labelMar 1, 2025
@github-actions
Copy link

👋 Hi,@neznaika0!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref:Syncing Your Branch

@neznaika0neznaika0force-pushed thefeat/improve-registrars branch fromff79dcb toc2eb405CompareMarch 8, 2025 09:36
@neznaika0neznaika0force-pushed thefeat/improve-registrars branch fromc2eb405 tof29a8c0CompareApril 8, 2025 20:06
@paulbalandanpaulbalandan removed the stalePull requests with conflicts labelApr 10, 2025
@github-actionsgithub-actionsbot added the stalePull requests with conflicts labelApr 20, 2025
@github-actions
Copy link

👋 Hi,@neznaika0!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref:Syncing Your Branch

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@samsonasiksamsonasiksamsonasik left review comments

Assignees

No one assigned

Labels

stalePull requests with conflicts

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@neznaika0@michalsn@paulbalandan@samsonasik

[8]ページ先頭

©2009-2025 Movatter.jp