- Issue created by @narendraR
- Status changed to Needs review
11 months ago 8:17am 22 May 2024 - First commit to issue fork.
- Status changed to RTBC
11 months ago 2:23pm 22 May 2024 - 🇺🇸United States smustgrave
Applied some nitpicky typehint return voids
Applied the MR to a 11.x install using standard profile
Believe the only entity_form_mode is for core.entity_form_mode.user.register
Ran updb and hook ran without issueManually downloaded config_inspector to 11.x setup and confirmed seeing fully validatable now
- Status changed to Needs work
11 months ago 12:57pm 23 May 2024 - 🇬🇧United Kingdom catch
One issue with the presave hook on the MR - missing a deprecation notice.
- Status changed to Needs review
11 months ago 10:54am 27 May 2024 - Status changed to Needs work
11 months ago 2:42pm 27 May 2024 - Status changed to Needs review
11 months ago 7:04am 28 May 2024 - Status changed to RTBC
11 months ago 12:59pm 28 May 2024 - Status changed to Needs work
11 months ago 2:34pm 28 May 2024 - 🇬🇧United Kingdom catch
One small comment on the deprecation message. Hard to fit this sort of thing into the standard format.
- Status changed to Needs review
11 months ago 4:07pm 28 May 2024 - Status changed to RTBC
11 months ago 4:20pm 28 May 2024 - Status changed to Needs review
11 months ago 9:53pm 28 May 2024 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Left one question on the MR, great work here - bit more involved with the update hook!
- 🇮🇳India narendraR Jaipur, India
This issue is ready for re-review. I have added the return FALSE. Thanks
- Status changed to Needs work
11 months ago 1:04pm 29 May 2024 - 🇺🇸United States phenaproxima Massachusetts
Looks okay but I have a couple of questions here.
- Status changed to Needs review
11 months ago 9:16am 3 June 2024 - Status changed to Needs work
11 months ago 2:49pm 3 June 2024 - 🇺🇸United States phenaproxima Massachusetts
Only one tiny thing and then I think this looks good.
- Status changed to Needs review
11 months ago 8:03am 5 June 2024 - Status changed to RTBC
11 months ago 1:45pm 10 June 2024 - 🇺🇸United States smustgrave
Applied MR on a standard install for 11.x with config inspector and getting all green checks.
Looking at MR 8135 and from what I can tell all feedback has been addressed.
- Status changed to Needs work
10 months ago 8:28am 27 June 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
If I go to admin/structure/display-modes/form/ and edit the user registration form and save the form mode with no description then the system_entity_form_mode_presave() will trigger a deprecation notice.
We need to add something to the form to convert empty strings to NULLs... normally we'd say use #config_target but we've not worked out how to using #config_target like stuff with config entities yet.
- Status changed to Needs review
10 months ago 12:40pm 3 July 2024 - Status changed to RTBC
10 months ago 7:21pm 3 July 2024 - 🇺🇸United States smustgrave
Tested the scenario in #21 has been addressed with https://git.drupalcode.org/project/drupal/-/merge_requests/8135/diffs?co...
- Status changed to Needs review
10 months ago 11:42pm 3 July 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
I'm not sure that a NULL description is any better then an empty string. Do we think that this part of the change is actually worth it?
- Status changed to Needs work
10 months ago 12:09pm 10 July 2024 The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- Assigned to wim leers
- 🇺🇸United States phenaproxima Massachusetts
I'm not sure that a NULL description is any better then an empty string. Do we think that this part of the change is actually worth it?
I don't feel strongly one way or the other, so I'm assigning this to Wim for input. I suspect he has a clearer reason why NULL would be useful here.
- 🇳🇱Netherlands bbrala Netherlands
Going through the config issues a bit to see where things are hanging.
I read through all this, and have not seen any discussion around the NULL change. I understand the text in the CR ("The reason is because an empty string makes no sense for this field. "" is never a useful description of a form mode."). But I dont really see why this is as much better as it is a change.
If i try to argue why it should be null: Now i want no desription is equal to en empty description. This unfortunately meant he field is not really optional, when you wouldn't post an description you technically tend a NULL, which cannot be stored right now. But if we allow null, we would be able to save without the field, since null is fine.
This could have reasons in jsonapi, maybe we can then post without the field and make it null, instead of requiring an empty string?
This is as far as i get.
- 🇳🇱Netherlands bbrala Netherlands
Seems fixtures might need work. Not sure how those work.
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
Found the tiniest of nitpicks, this is just a version number change in the deprecation, so leaving at rtbc.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
We need to move the hook implementation to the correct new OOP place... \Drupal\system\Hook\SystemHooks
- 🇳🇱Netherlands bbrala Netherlands
Thanks, you are right. By now we have the OOP hooks. I think this issue is present in the other validation issues also. I'll do a round.
- 🇳🇱Netherlands bbrala Netherlands
I think this change is so minimal i can RTBC again. I ran the upgrade test after moving the hook code to SystemHooks.php and it upgraded fine.
-
alexpott →
committed 4f3432bf on 11.x
Issue #3448457 by narendrar, bbrala, smustgrave, alexpott, phenaproxima...
-
alexpott →
committed 4f3432bf on 11.x
- 🇳🇱Netherlands bbrala Netherlands
CR was out of date regarding versions, does it need publishing also? Probably?
- 🇳🇿New Zealand quietone
I'll take the 'probably' as a 'yes'. I have updated and published the CR.
- 🇳🇱Netherlands bbrala Netherlands
Hehe yeah, it would need publishing, ty. Didn't want to do it myself for possible text issues.
- Issue was unassigned.
- Status changed to Fixed
5 days ago 5:54am 25 April 2025 Automatically closed - issue fixed for 2 weeks with no activity.