#Novice

Was the bot actually wrong?

Yeah, it merged cleanly. I have seen it a handful of times.

Was the actually wrong?

We probably want to keep the bot here so we can ensure this is up to date when committers get to it.

I reviewed everything that gitlab showed as changed since my review yesterday rebase seems good.

Thanks for looking that up! Changes LGTM

I guess since Hooks classes are now autoloaded unconditionally it doesn't make sense to check if a method exists. I've updated the test to check if the hook implementation exists instead.

The assertion was added in #2160091-61: drupal_rebuild() rebuilds container twice, since drupal_flush_all_caches() also rebuilds it → (specifically comment #61). Have not reviewed closely, just identifying where it happened.

Can you please review MR!12

nathankg → closed merge request !11

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.

Summary doesn't appear to match the MR. Should check the issue the assert was added to see what the goal was.

Looks great!

From the code context, it seems like the usage of that assert is to confirm system_test module is not installed. If there is an issue with checking hooks, then we can do the same assertion using module_handler's moduleExists function. Plz review the code.

longwave → closed merge request !11459

I could argue that helpfuLinks() should be filtered after the fact, maybe the links template could have an option to check access before rendering each link, but this works for me as a simple solution given there are only two links to check.

Committed 9a1d0b1 and pushed to 11.x. Thanks!

Rebase done and tests moved fixed, but a $this->drupalLogout() from a previous tests is failing because Mink can't find "op" button.

https://git.drupalcode.org/project/experience_builder/-/jobs/5801240#L976

After inspecting the HTML right there, the user is logged in and there is a link for logout.

        <ul>
          <li>
            <a href="/user" data-drupal-link-system-path="user">My account</a>
          </li>
          <li>
            <a href="/user/logout?token=INyOF8qdzpnT1xzUapL9vdyU4YKpjP1WM6X-JZOG-VM"
               data-drupal-link-system-path="user/logout">Log out</a>
          </li>
        </ul>

I also added a $this->drupalLogin($this->httpApiUser) and it also fails.

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

Trivial — and I bet the CI failures here were random. Merged in upstream to verify that any failures match recent random failures.

XB now requires 11.2 so I believe this can be actioned.

I think this is ready again.
I pulled it down and confirmed there are no more uses or comments about these constants.

I confirmed the three enums values and cases align with the original constants.
The deprecations look right.

In this check I did the following for each file.

1. Check what the module is or what it is related to, comment, link, or node.
2. Confirm that the correct enum was loaded.
3. Confirm each replacement aligned with the correct constant and case.
4. Hit the checkbox to minimize the file and moved on to the next.

Updated the issue summary as requested as well.

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

OK that test fail suggests this might not be a novice issue. IIRC there is another issue for ignoring hooks from modules that are not installed. This might be a duplicate of that.

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

I think those look great, thanks @nexusnovaz!

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

Saving credits according to our core issue credit guidelines → . Thanks!

I've added some descriptions, i think this is good for review.

Thanks.

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.

Great! Thanks @nexusnovas and @lostcarpark 😃

-mike

I have reviewed the change, and checked the codebase for any other duplicate instances of <dl> <dt> <dd>, and didn't find any.

I did not carry out a manual test, but I don't think it's required.

I think this is ready for RTBC.

Crediting issue triage participants.

I've found and removed the duplicated html elements. Please review MR !21

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

If the goal is to get Drupal to cache the response, then I think the entire changeset is necessary.

If the goal is to only make the response theoretically cacheable, then I am willing to revert the changes related to $is_daylight_saving_time and open a separate issue to address the PHP notice.

I've faced a somehow related issue with Floursish oEmbeds, not in terms of aspect ratio but in terms of the maxwidth/maxheight not overriding the default height and weight, for example:

https://app.flourish.studio/api/v1/oembed?url=https://public.flourish.st...

Should display a width of 960px but it shows the default 700px as it is forced by the weight attribute in the returned JSON file.

Adding a solution like #27 for the type_rich would be my suggestion to solve this.

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

Updated the issue summary as suggested.

Still wondering if the solution is a little out of scope of the issue summary

Sorry, did not realize version 8.x-2.x has already the README.md file. I'll close this as works as designed

Left a question on the MR

Appears to have some open threads, 1 for discussion and 1 for action item.

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

Hi,

I have verified this issue, on the D11 version. This is working fine for me.

Testing steps:

1. Edit Article content type
2. Navigate to manage form display
3. Add nested field groups, move one mandatory field within the nested group
4. Add/Edit the article content
5. Mandatory Asterisk will be missing for Nested field groups

Testing Results:

• Applied the patch successfully, Asterisk mark is showing properly for the Nested field group.

Hence this can be moved to RTBC +1

Attaching screeenshots for reference.

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

acbramley → closed merge request !12337

xjm → credited alexpott → .

xjm → credited catch → .

xjm → credited star-szr → .

Adding credits for the triage from #8.

@berdir flagged in #22

there are also node_comment hooks that update the search index

They are as follows:

1. \Drupal\node\Hook\NodeHooks1::commentInsert
2. \Drupal\node\Hook\NodeHooks1::commentUpdate
3. \Drupal\node\Hook\NodeHooks1::commentDelete

But comments only reference a node by entity-id, not entity and revision ID, so in that scenario I think we're always going to have the default revision

Setting back to needs review for someone to confirm my thinking on that

Updated the proposed resolution to deprecate instead of adding to the interface.

Couple of questions on the MR, keeping at RTBC for now

One is an obvious typo that I'll self apply

Credits

Great work, left some feedback on the MR - but this is pretty close 💪

Latest patch has far too much detail. And reads even more like it's LLM-generated.

This issue is just about documenting the bundles.

Created Patch