-
-
Notifications
You must be signed in to change notification settings - Fork 969
7.1.x Scaffolding Display Constraint Expansion #15266
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
base: 7.1.x
Are you sure you want to change the base?
Conversation
|
Looks useful! I find the |
There already is an editable constraint that controls whether a field is read-only in forms. "input" and "output" directly correspond to the view types:
Using I chose NONE because it is similar to display: 'none' is a css rule which is pretty common.
|
| * @return The display type controlling where this property is shown in scaffolded views | ||
| * @since 7.1 | ||
| */ | ||
| DisplayType getDisplayType() |
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.
So to date, everything has been an optional / minor breaking change. But changing constrained means that this will be a breaking change, no? @matrei what are your thoughts on this?
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.
@jdaugherty is adding a constraint breaking?
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.
Adding the constraint isn't, but changing the base interface that's applied to every object I assume would be breaking. We need to test this.
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.
@sbglasius is going to test this change with his side project to ensure it's backwards compatible.
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.
Yes, we should test this, we don't want to have to recompile/re-release plugins for 7.1 compatibility.
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.
Turns out, I'm implementing grails.gorm.validation.Constraint and not grails.gorm.validation.Constrained
I think it would be best, if @codeconsole makes a Constrained implementation with 7.0.4 and run it with 7.1.0-SNAPSHOT
Or direct me on how to do so.
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.
@sbglasius I think the real concern is this being a breaking change across domain objects for 7.x.
If you have your plugin with a domain class on 7.0.4, and you have your application on 7.1, what happens when you call findConstrainedProperties(Holders.grailsApplication.mappingContext.getPersistentEntity(MyDomain.class)) in your application? If it works, this isnt' a breaking change, if it doesn't, it is.
grails-datamapping-validation/src/main/groovy/grails/gorm/validation/DisplayType.groovy
Show resolved
Hide resolved
grails-fields/src/main/groovy/org/grails/scaffolding/model/DomainModelServiceImpl.groovy
Show resolved
Hide resolved
@codeconsole Yes, that's a valid point. I'm fine with the naming as it is. |
|
My only remaining concern is if this is a breaking change. If it is, we need to agree as a group that this is ok for a minor release since it will force all plugins to be recompiled again. |
Modify display constraint so that
For instance, let's say you have
String passwordyou would wantdisplay: INPUT_ONLYFor
Date dateCreated, perhaps ideal configuration isdisplay: OUTPUT_ONLYThis is backwards compatible with
display: boolean