-
Notifications
You must be signed in to change notification settings - Fork 453
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Express Form controller #7014
Improve Express Form controller #7014
Conversation
72f4463
to
5c23665
Compare
If someone could review these commits, I'd be really thankful! |
/** | ||
* @return array [ | ||
* Key: string, asset type | ||
* Value: string, the URL of the asset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will return Value: array of strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor and code cleanup.
@@ -514,7 +510,7 @@ public function save($data) | |||
$key->setAttributeType($type); | |||
$settings = $control->getAttributeKey()->getAttributeKeySettings(); | |||
$settings->setAttributeKey($key); | |||
$settings = $settings->mergeAndPersist($entityManager); | |||
$settings->mergeAndPersist($entityManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You sure about this change? mergeAndPersist specifically returns a value that is now merged in the entity manager:
$detachedEntity = unserialize($serializedEntity); // some detached entity
$entity = $em->merge($detachedEntity);
Please change this pull request so that it only makes the obvious changes like removing needless facades, actual code styling cleanup, and removing unused imports, etc... Anything else and you might be misunderstanding what the original code does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it because $settings
isn't used later on in the code (afaik). I'll add it back if that's what you prefer.
PR:
app
container instead of Core facadePage::getCurrentPage()
view
method to the top, because that's where you'd expect itI consider this as a first round of optimization / code clean up...
Ref #4723