8000 Improve comment about View\ModelTrait::$model DI in View::init() by PhilippGrashoff · Pull Request #2290 · atk4/ui · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Improve comment about View\ModelTrait::$model DI in View::init() #2290

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
May 16, 2025
Merged

Improve comment about View\ModelTrait::$model DI in View::init() #2290

merged 5 commits into from
May 16, 2025

Conversation

PhilippGrashoff
Copy link
Collaborator
@PhilippGrashoff PhilippGrashoff commented May 11, 2025 •
edited by mvorisek
Loading

fix #2284

This PR removes some logic inside View::init() that was maybe kept for backwards compatibility, as View used to have a model property until v6.
As View now does not have a model property any more, it also shouldn't include the logic to call setModel() method if a model property exists. This should be handled by classes extending View which add a model property.

@mvorisek mvorisek changed the title View: remove code executing setModel() if $this->model exists Remove View::$model DI from View::init() May 11, 2025
@mvorisek
Copy link
Member

Currently one demo relies on this: demos/_unit-test/scope-builder.php. Easy to fix.

However, what is the motivation of this change? Clean OOP?

I am very worried about migration path. I am fine with BC-break, but the main BC-breaks should be at least easily detectable using PHPStan l6 or higher. This is not the case here.

We can move this into View\ModelTrait, but then we would need to handle overriden init() conflicts.

@mvorisek mvorisek marked this pull request as draft May 11, 2025 12:16
@mvorisek mvorisek mentioned this pull request May 11, 2025
Closed
@mvorisek
Copy link
Member

If DI with model property is minimal, we can override setDefaults (to not require having to resolve conflict of init) and throw there if the model (and entity) property was set not thru setModel/setEntity. This way, PHPStan will still not detect any issue, but runtime will.

Wdyt?

@PhilippGrashoff
Copy link
Collaborator Author

Hi,

motivation for this PR - clearly clean code. However, I looked at the code in View::init() isolated from ModelTrait, which it clearly "partners" with.

  • The cleanest solution would be to have a ViewWithModel class extending View having both the init() logic as well as ModelTrait content. But as many classes using ModelTrait already inherit ViewWithContent, this unfortunately is not an option.
  • If the code I removed checked if ModelTrait was used, i'd be ok with it, Then, it would a) be clear that this code is meant to partner with ModelTrait and b) would only execute if ModelTrait was actually used. In the current implementation, you can extend View, add a $model property and then be surprised about the exception that setModel() is not defined :)
  • The alternative would be to force users to use ModelTrait in case a $model property is added to a child class of View.
  • regarding overwriting setDefaults: Can you explain what you mean? I do not 100% get it yet.

@mvorisek
Copy link
Member

If the code I removed checked if ModelTrait was used, i'd be ok with it, Then, it would a) be clear that this code is meant to partner with ModelTrait and b) would only execute if ModelTrait was actually used. In the current implementation, you can extend View, add a $model property and then be surprised about the exception that setModel() is not defined :)

Here you landed several good points and I think the only solution is to move the code into ModelTrait directly to address all of them easily.

As long as the code can be put directly after parent init, it can be coded quite easily, let me to check this later.

@mvorisek
Copy link
Member

I really do not know how to fix this.

08bfb91 is not good solution as then traits with multiple init could not be combined.

And TraitUtil from core is not advised to be used in general - https://github.com/atk4/core/blob/6.0.0/src/TraitUtil.php#L31.

So maybe the updated comment seems to be fine fo "clearly clean code" motivation.

Situation when user adds model property is not rare, but he should use the View\ModelTrait, so in reality, there is no issue.

@mvorisek mvorisek removed the BC-break label May 12, 2025
@mvorisek
Copy link
Member

08bfb91 is not good solution as then traits with multiple init could not be combined.

We would need phpstan/phpstan#13010 first.

@mvorisek mvorisek changed the title Remove View::$model DI from View::init() Improve comment about View\ModelTrait::$model DI in View::init() May 16, 2025
@mvorisek mvorisek marked this pull request as ready for review May 16, 2025 19:53
@mvorisek mvorisek merged commit e65f783 into atk4:develop May 16, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

View::setModel() called in View::init() - still needed in V6?
2 participants
@PhilippGrashoff @mvorisek
0