Remove hook_node_validate() and hook_node_submit() because they bypass the entity API

We had one case where we had to use this hook. It is currently not possible to add constraints to a configurable field. We have a field that is just a simple string field, but it has to be unique. It would be annoying if we have to define a field type just for that, as we then also have to alter widgets and formatters to make them available.

My example was from a custom project, but yes, forum is similar.

Yes, it is a general problem, but it is also a blocker for removing this hook ;)

Personally, I'm fine with it either way, just wondering if we can justify an API change for removing it completely, or should we mark it as deprecated and say that you should use constraints instead?

We probably can, because security and stuff, but I still wanted to ask.

pretty sure forum_node_validate is removed in #2247085: Constraints cannot be added to configurable fields (party)

I agree with @fago - we should remove hook_node_validate() and hook_node_submit()

A starter for 10.

+++ b/core/modules/menu_ui/menu_ui.module
@@ -354,14 +354,16 @@ function menu_ui_form_node_form_alter(&$form, FormStateInterface $form_state) {
     '#description' => t('Menu links with lower weights are displayed before links with higher weights.'),
   );
+  array_unshift($form['actions']['submit']['#submit'], 'menu_ui_form_node_form_submit');
...
-function menu_ui_node_submit(EntityInterface $node, $form, FormStateInterface $form_state) {
+function menu_ui_form_node_form_submit($form, FormStateInterface $form_state) {
+  $node = $form_state->getFormObject()->getEntity();
   if (!$form_state->isValueEmpty('menu')) {

not sure if submit belongs in this issue.

But when we touch it, then should be able to clean this up so that we don't have to attach $node->menu to the node anymore if we add the submit callback last and not at the beginning?

I searched for node_submit and there is core/modules/node/src/Tests/NodeSaveTest.php with a strange "Function node_submit() preserves user ID", so we should probably also fix that comment in this patch.

I searched for node_validate and core/modules/system/entity.api.php still mentions it, so that should be fixed, too.

But otherwise the patch looks good. I think we should not try to also fix $node->menu in this critical issue, that can be a follow-up.

And we will need a change notice draft with instructions what developers should use instead.

Was very hard to stop with the menu_ui cleanup ;)

+++ b/core/modules/menu_ui/menu_ui.module
@@ -127,54 +127,33 @@ function menu_ui_block_view_system_menu_block_alter(array &$build, BlockPluginIn
+function menu_ui_node_save(NodeInterface $node, array $definition) {

@@ -196,70 +175,73 @@ function menu_ui_node_predelete(EntityInterface $node) {
+function menu_ui_get_definition(NodeInterface $node) {

this so feels like we want a hook_entity_base_field_info like path module and define a 'menu_link' ER field. Perhaps a follow up? Would be so much cleaner.

1. +++ b/core/modules/node/src/NodeForm.php
   @@ -293,14 +293,6 @@ public function validate(array $form, FormStateInterface $form_state) {
   -    // Invoke hook_node_validate() for validation needed by modules.
   -    // Can't use \Drupal::moduleHandler()->invokeAll(), because $form_state must
   -    // be receivable by reference.
   -    foreach (\Drupal::moduleHandler()->getImplementations('node_validate') as $module) {
   -      $function = $module . '_node_validate';
   -      $function($node, $form, $form_state);
   -    }
   

   Yay ! Any chance we can stop doing $node = $this->buildEntity($form, $form_state); a couple lines above then ?
   See #1768526: NodeFormController::validate() calls buildEntity() twice :-)

2. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -127,54 +127,33 @@ function menu_ui_block_view_system_menu_block_alter(array &$build, BlockPluginIn
   + * Helper function to save a menu link for a node.
   ...
   +function menu_ui_node_save(NodeInterface $node, array $definition) {
   

   Nitpick : if it's a helper, can we prefix it with '_' ?
   The current name still looks damn close to a hook implementation :-)

3. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -196,70 +175,73 @@ function menu_ui_node_predelete(EntityInterface $node) {
   +function menu_ui_get_definition(NodeInterface $node) {
   

   Nitpick: the function name isn't very clear on "definition of what ?"

3. That's kind of the problem... menu_ui_get_weird_definition_that_is_somehow_supposed_to_represent_a_menu_link() ? ;) I kept that "definition" thing, but it really is pretty strange. menu_ui_get_menu_link_definition() doesn't make it much clearer? maybe menu_link_defaults() would be better? (which resembles the hook that we had some time ago ;))

1. Hm. We still have the changed validation there, but we should be able to fix that by returning $entity in parent::validate(), just like we already do for save()?
2. Added _, also renamed $definition there to $values.
3. Renamed to menu_ui_get_menu_link_defaults() as per my last suggestion, also renamed the relevant variables. Even more unrelated changes in this issue :)

We still have the changed validation there, but we should be able to fix that by returning $entity in parent::validate(), just like we already do for save()?

Right, I missed the $node->getChangedTime() at the very end of the line :-/

That sound like a plan, yes, it feels silly to have each override of validate() re-do a buildEntity() because the results are lost.
Probably more a task for #1768526: NodeFormController::validate() calls buildEntity() twice than for this patch here, though :-)

Looks good, I had to review with git diff --color-words, most of the changes are just variable renaming. One nitpick left then we only need the change notice draft.

+++ b/core/modules/system/entity.api.php
@@ -152,7 +152,6 @@
  * Some specific entity types have additional hooks that are run during
  * various steps in the process:
- * - Node entities: hook_node_validate() and hook_submit().

Now the sentence above does not make sense with the ending ":" ;-)

Hehe, nice.

Some specific entity types have additional hooks...

No, they don't. At least nothing that's worth documenting here. So I just removed it...

Started with a CR: https://www.drupal.org/node/2420295

Yay. Looks RTBC to me then.

RTBC+1, I updated the change notice with an example D7 version for hook_node_submit() (which does not make completely sense, but it should work to demonstrate the conversion).

I don't think a #submit builder is a correct alternative to the #entity_builder, i.e. it would be missed from EntityForm::buildEntity() and thus during validation. Thus, I removed that "alternative" from the change record.

I disagree. Yes, #submit is not an alternative to #entity, but that is not what I said (or meant). But that hook_node_submit() should be converted to entity builder or #submit. This patch uses #submit, and it is correct in doing so. If you want to *do* something with the created entity, use #submit, if you want to help building it before saving, use #entity_builder.

If that was not clear enough then we need to improve it, but removing it is wrong.

OK. Committed/pushed to 8.0.x, thanks!

I think we have an issue somewhere to look at the menu links on node form (or that's one of the intended follow-ups from the NJ sprint at least), so we can revisit that code in there.

I think this is the issue for the menu_links on the node form: #2315773: Create a menu link field type/widget/formatter.

Thanks, restored and extend my initial example in the submit callback, to get the $node object and doing something with it, which is I think something that might be more common than getting values from $form_state, and people know how to do that.

This may have changed a UI behavior.