Commited and pushed to both 2.x and 3.x
borisson_ → made their first commit to this issue’s fork.
This looks correct, the 3.x (and especially this submodule) is not yet stable, so we're going to add this once we've settled. Will keep this issue open to resolve that when the time is there.
I reviewed it again today as well, and the one remark I had was answered. RTBC++
larowlan → credited borisson_ → .
Committed and pushed to 2.x and 3.x, thanks everyone!
quietone → credited borisson_ → .
It will be hard to keep this in mind while reviewing config schema issues, but since it's a simple set of rules that are easily documented we should proceed like this, I think #6 tackles my concerns.
Most things in system.site that are being made nullable in 📌 Add validation constraints to system.site Needs work , are not actually nullable from the standpoint of a running drupal installation, this especially rings true for name and UUID.
However, I'm not so sure about others. What is the semantic meaning of page.403 / page.404 being an empty string. It means this is not configured, right? I think it makes more sense in that case for it being NULL, because we can say it has to be null or it has to be a valid path.
If it is not nullable, we can not be as strict on the shape of the value?
page: 403: '' 404: '' page: 403: null 404: null
I think this will reduce the need for nullable and means that we can leave values as strings and not have type flip-flopping.
Type flip-flopping is not great. and it doesn't make our code more strict.
All it does, is allows us to assume if the value is a string, it's a valid value, but I would not like to see a lot of places introduce is_string as checks to see if it's the right value.
I don't yet have a fully formed opinion on what the best way forward is.
I've opened #3461428: Add NotBlankAfterInstall constraint and use it for system.date:timezone.default to explore the NotBlankAfterInstall idea.
I think this is a very valueable new constraint.
Commited and pushed to both 2.x and 3.x, thanks!
Merged, thanks so much.
We no longer have the array of ignore things in ChemaCheckTrait, as far as I can see, so I think you're right that this can be closed.
Looks great, thanks all.
Thanks, pushed and committed to 2.x, can we can get a 3.x variant of the change as well?
Spellcheck fails, but not sure how to better name this.
dan2k3k4 → credited borisson_ → .
borisson_ → created an issue.
This is not the way, the way is in search api
Back to rtbc now that the remarks have been fixed.
This requires us to add something like EntityTypeExistsValidator, currently working on that.
borisson_ → made their first commit to this issue’s fork.
Merged in these changes, should we open a new branch for 3.x?
This looks great. I really like the test coverage provided here. That looks like it covers all possible combinations. I think we should wait for 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work to get in.
Hurrah, it's green.
I think the mr needs to be rerolled?
Followup created: 📌 Add PHP_INT_MAX to integer schema configuration Active
borisson_ → created an issue.
The remaining failures are all in rest, but I don't understand how to fix them.
Discussed with @alexpott at drupal dev days, setting default values on the entity.
We now have specific test coverage for a type that is null. This looks good to me.
Changes look good to me. RTBC.
I think this looks great, this makes it easier for other issues to land. Marking as rtbc.
Looks like a lot of the current failures are because of invalid configuration in tests.
This was fixed in another issue.
Committed to 2.x, not sure if we need this for 3.x also.
Thanks!
borisson_ → made their first commit to this issue’s fork.
Committed to 2.x and 3.x, thanks!
Merged to both 2.x and 3.x, thanks!
borisson_ → changed the visibility of the branch 3342656-facets-searchbox-widget to active.
This looks great, but I think we should probably add a test or it also?
I think this solution is not the most readable, it can be rewritten to be clearer. It should also come with a test where we can prove this issue is actually fixed.
While I think this is interesting functionality, we're currently not at the stage where we want to add more to the facets module, especially without rigorous testing. I think we should create this as a contrib extension instead, we can link to it from the readme.
I think this is a good idea if we would've seen this closer to when this was added, since it has been a while now, I think it will only lead to trouble and it just a cosmetic fix. I think therefor we should just won't fix this issue.
I think it was just an oversight that this was not done from the start, will commit to both 2.x and 3.x
Committed to both branches.
This looks great, I think we should commit it to be 2.x and 3.x
Committed to the 2.x branch also.
bbrala → credited borisson_ → .
Committed to both 2.x and 3.x
quietone → credited borisson_ → .
Thanks for the useful feedback!
borisson_ → created an issue. See original summary → .
I love the new test coverage here, this is really good. I had one remark on the type definition, but I'm not sure if that would be better.
I think this looks great, I agree that both html and regex being new types is a good idea for reusability. We should create change records for both of those new types.
borisson_ → created an issue.
The readme in the folder looks good.
bbrala → credited borisson_ → .
I agree, let's explicitly disallow this.
borisson_ → created an issue.
borisson_ → created an issue.
borisson_ → created an issue.
borisson_ → created an issue.
borisson_ → created an issue.
borisson_ → created an issue.
borisson_ → created an issue.
BramDriesen → credited borisson_ → .
longwave → credited borisson_ → .
BramDriesen → credited borisson_ → .
BramDriesen → credited borisson_ → .
Setting to rtbc to commit to 2.x
Do we consider this to be a BC break and as such can it only go into 3.x?
Added as a supporter
I could add keys to UserMailNotifyTest and remove the requiredkey: false from most of them
I agree, this looks like the best way
larowlan → credited borisson_ → .