- 🇦🇺Australia acbramley
Was the bot actually wrong?
Yeah, it merged cleanly. I have seen it a handful of times.
- 🇺🇸United States nicxvan
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.
- 🇦🇺Australia mstrelan
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.
- 🇦🇺Australia mstrelan
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.
- @nathankg opened merge request.
- @nathankg opened merge request.
- First commit to issue fork.
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 States smustgrave
Summary doesn't appear to match the MR. Should check the issue the assert was added to see what the goal was.
- 🇮🇳India sijumpk
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.
- First commit to issue fork.
- 🇬🇧United Kingdom longwave UK
I could argue that
helpfuLinks()
should be filtered after the fact, maybe thelinks
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!
-
longwave →
committed 9a1d0b13 on 11.x
Issue #3512116 by annmarysruthy, juandhr, larowlan, mstrelan, quietone...
-
longwave →
committed 9a1d0b13 on 11.x
-
wim leers →
committed 03e5fbe4 on 0.x authored by
thoward216 →
Issue #3526721 by thoward216, wim leers, catch: Require Drupal 11.1.8...
-
wim leers →
committed 03e5fbe4 on 0.x authored by
thoward216 →
- 🇪🇸Spain isholgueras
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.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Trivial — and I bet the CI failures here were random. Merged in upstream to verify that any failures match recent random failures.
- First commit to issue fork.
- @thoward216 opened merge request.
- 🇬🇧United Kingdom thoward216
XB now requires 11.2 so I believe this can be actioned.
- First commit to issue fork.
- 🇺🇸United States nicxvan
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.
- 🇦🇺Australia mstrelan
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.
- @guri221 opened merge request.
- First commit to issue fork.
- Issue created by @mstrelan
- Issue created by @nexusnovaz
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States xjm
Saving credits according to our core issue credit guidelines → . Thanks!
- 🇬🇧United Kingdom nexusnovaz
I've added some descriptions, i think this is good for review.
Thanks.
- First commit to issue fork.
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.
- @chhavisharma opened merge request.
- 🇺🇸United States ultimike Florida, USA
Great! Thanks @nexusnovas and @lostcarpark 😃
-mike
-
ultimike →
committed 3840b335 on 2.0.x authored by
nexusnovaz →
Issue #3533956 by nexusnovaz, lostcarpark: Duplicate tags in schema
-
ultimike →
committed 3840b335 on 2.0.x authored by
nexusnovaz →
- 🇮🇪Ireland lostcarpark
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.
- 🇬🇧United Kingdom nexusnovaz
I've found and removed the duplicated html elements. Please review MR !21
- @nexusnovaz opened merge request.
- First commit to issue fork.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States mediabounds
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. - Issue created by @ultimike
- 🇪🇸Spain pcambra Asturies
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.
- 🇺🇸United States smustgrave
Still wondering if the solution is a little out of scope of the issue summary
- 🇨🇦Canada danrod Ottawa
Sorry, did not realize version 8.x-2.x has already the README.md file. I'll close this as works as designed
- Issue created by @danrod
- 🇺🇸United States smustgrave
Appears to have some open threads, 1 for discussion and 1 for action item.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇮🇳India roshanibhangale
Hi,
I have verified this issue, on the D11 version. This is working fine for me.
Testing steps:
- Edit Article content type
- Navigate to manage form display
- Add nested field groups, move one mandatory field within the nested group
- Add/Edit the article content
- 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.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
@berdir flagged in #22
there are also node_comment hooks that update the search index
They are as follows:
- \Drupal\node\Hook\NodeHooks1::commentInsert
- \Drupal\node\Hook\NodeHooks1::commentUpdate
- \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
- 🇦🇺Australia mstrelan
Updated the proposed resolution to deprecate instead of adding to the interface.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Couple of questions on the MR, keeping at RTBC for now
One is an obvious typo that I'll self apply
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Great work, left some feedback on the MR - but this is pretty close 💪
- 🇬🇧United Kingdom joachim
Latest patch has far too much detail. And reads even more like it's LLM-generated.
This issue is just about documenting the bundles.
- @ankusht1515 opened merge request.
- First commit to issue fork.
- @viragjain opened merge request.