#Novice

Hi @catch
Is the file name "nodehooks1"?
I couldn't find it. Could you tell me where it is located?

I’ve fixed the code standards issues.

Automatically closed - issue fixed for 2 weeks with no activity.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Committed 93ec7a9 and pushed to 11.x. Thanks!

nod_ → closed merge request !7200

all good, thanks

rinku jacob 13 → made their first commit to this issue’s fork.

It's not a 100% duplicate, e.g. query_node_access_alter only appears to be in this file. Might be necessary to merge it with NodeHooks.

I've left a few code change suggestions and comments in the MR. We should then also have some tests for this.

There is a phpcs failure, needs to be fixed before I can commit

Hello @charlliequadros
I have checked the MR and it seems that the suggested changes have been implemented

Moving the issue to RTBC and let's wait for some more feedback from @alexpott or @longwave,

Hi @alexpott, @longwave,
I re-added the comment, fixed the typo, and submitted it for review again.

Committed 59623d9 and pushed to 11.x. Thanks!

nod_ → closed merge request !11202

Committed 4dcbb64 and pushed to 11.x. Thanks!

nod_ → closed merge request !9615

Committed e7a6df2 and pushed to 11.x. Thanks!

nod_ → closed merge request !6723

Updated the MR to reflect comment #34 and earlier.

TODO: See comment #4, points 1 and 4 (at least).

what's left here to do? maybe just a change record?

Since it's just removing the file, going ahead from needs review here (would have been OK to restore the RTBC).

Committed/pushed to 11.x, thanks!

Since there hasn't been a follow up going to close out for now. If still an issue please reopen.

Hi @penyaskito,
I resolved the conflict in the MR.

I improved the issue summary, hope it makes more sense.

Hi @sivaji,
I made the change to organize it in the correct granularity order.

Needs a rebase.

Committed 7258763 and pushed to 11.x. Thanks!

nod_ → closed merge request !11275

Hi @joachim,
it's not clear what the comment is meant to convey. To me, it should include something like:

// If $form_state->isRebuilding() was set by $form_state->setRebuild() and the input was
// processed without validation errors, we are in a multi-step workflow
// that is not yet complete. In this case, a new $form needs to be constructed based on
// the changes made to $form_state during the request.

Feedback from @quietone appears to be addressed

Have not reviewed but issue summary appears incomplete

Automatically closed - issue fixed for 2 weeks with no activity.

Thanks @berdir , good to know, I'll do that next time.

tom konda → made their first commit to this issue’s fork.

Created MR to reflect the above comments.

All contributions must be merge requests.

I attached a patch that checks for isDefaultRevision() being set to TRUE before running node_reindex_node_search and same situation for commentInsert or commentUpdate and or commentDelete at /core/modules/node/src/Hook/NodeHooks1.php

I'm starting to do some Core contribution so bear with me when reviewing.

The failing test Drupal\Tests\file\Functional\DownloadTest is passing for me locally with this MR, and I don't see how it can be related anyway. Must be some glitch in the testing system.

Hi @joachim,
I made the change you requested, assigning null to the second parameter to make the code clearer and easier to understand when read. It seems like there's an issue, but I'm not sure how to fix it.
I'm just starting to contribute, and I would really appreciate your advice on how to resolve this.

ankitv18 → changed the visibility of the branch 3422332-fix-gitlab-pipeline to hidden.

Thanks for the help on this one

Left a comment on the MR.

I’ve adjusted the docblock formatting to ensure compliance with Drupal coding standards.

Thank you for your help!

> // If $form_state->isRebuilding() returns TRUE and input has been processed

This is correct and accurate, but from the POV of DX, it doesn't fix the problem.

The original docs approach this from answering the question, 'what do you need to do to the form to get this behaviour?'

> If $form_state->isRebuilding() has been set

It's just that the technical detail is wrong.

We need to change the detail, not the meaning being conveyed.

Thanks!

But all the @return docs must be a single paragraph -- see https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... → .

Thanks for the help with this one.

Thanks for the feedback! I’ve removed the unsupported @note tag, kept the explanation in the standard comment format and fixed the coding standards.

@joachim, noted. Thank you!

The problem was that it needed a rebase -- 11.x had a new feature for week granularity, and the test for must have got pulled in by merging 11.x into the branch and not properly resolving the merge conflict.

I've rebased and resolved the merge conflict and the test passes now.

Please post text rather than screenshots! I can't copy a class name from a screenshot!

The wording is good, but @note is not a docs tag supported in the list at https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... →

Patch to be updated for core policy change to meet the deprecated code removal → .

Hey everyone,
I've already made the initial changes and tested locally.Now I'm preparing the fork and will push the changes shortly.