#Needs release manager review

It is used to alert the release manager core committer(s) that an issue significantly affects the overall technical debt or release timeline of Drupal, and their signoff is needed. See the governance policy draft for more information.
โšก๏ธ Live updates comments, jobs, and issues, tagged with #Needs release manager review will update issues and activities on this page.

Issues

The last 100 updated issues.

Activities

The last 7 days of comments and CI jobs.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    I think this looks good! And good timing

  • ๐Ÿ‡ช๐Ÿ‡ธSpain fjgarlin

    Link added. The link points to the 11.x page where the file does not exist yet.
    https://git.drupalcode.org/issue/drupal-3399348/-/blob/3399348-svg-templ...

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Brought up in #needs-review-queue-initative in slack. Was mentioned by @catch that

    detection logic is still in progress

    and that parts will need the release manager review.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    I think a link to the gitlab page would work.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    In gitlab it looks perfect.

  • ๐Ÿ‡ช๐Ÿ‡ธSpain fjgarlin

    I only used https://mermaid.live, and the result on Gitlab, not any local dev tools.

    Same here. I think that seeing just the code in the editor is fine.

    In order to render it locally, we'll need a plugin in the editor.

    Maybe we can add a note inside the md file above or below the graph saying that mermaid is supported by GitLab and that to view it locally you'd need a plugin, but not sure that it is needed. ie: even if markdown viewer is very common in most editors, it's not always present.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    using that plugin this is what I see

  • ๐Ÿ‡ฉ๐Ÿ‡ฐDenmark ressa Copenhagen

    Have you found a way to integrate https://github.com/mermaid-js/mermaid in PHPStorm @smustgrave? Because I only used https://mermaid.live, and the result on Gitlab, not any local dev tools.

    @fjgarlin: Could you render the diagram locally?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Still just seeing code

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    I've read it should be possible to use a Docker-based solution to run Linux with Apache or Nginx and PHP on a Windows host. In such a scenario, Drupal may not be relying directly on Windows, but it would still be running on a Windows server.

    This would still be supported more or less by default because we support running Drupal on linux in docker, and what exactly docker runs is not usually taken into account. As you say:

    In a Docker-based scenario, chances are, any security issue would be more related to the architecture within Docker rather than the host anyway.

    Agreed with this - I can't think of a situation where there'd be an issue, especially a security issue, specific to Drupal on docker on windows, as opposed to a generic Drupal issue or a generic Docker issue.

    However dropping support for directly running on Windows in production also means dropping support for apache on windows, not just IIS on windows - just to make that clear. We'd still accept bug reports from people using windows + apache (or even IIS) for local development, but any security issue would be made public as long as it's really windows-only, and we'd stop shipping the IIS file.

    I tried to update the issue summary to make this a bit clearer, although the wording probably needs improvement.

  • ๐Ÿ‡ช๐Ÿ‡ธSpain fjgarlin

    Back to Needs review I assume.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Not sure if I did something wrong but applied the MR and viewing the file I still see just the code and not the markdown.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States paulmckibben Atlanta, GA

    I have, in the past, supported a D7 site on Windows/IIS, and I've had inquiries about the possibility of running a corporate-internal D10 site on Windows. This is to say, there are still a few organizations out there who run Windows server networks internally, but still have an interest in using Drupal and other open source software.

    I think it's fine to drop IIS support. However, I think we need to distinguish that from "Windows" support. For example (and I haven't tried this), I've read it should be possible to use a Docker-based solution to run Linux with Apache or Nginx and PHP on a Windows host. In such a scenario, Drupal may not be relying directly on Windows, but it would still be running on a Windows server.

    • What I'd like to see is a conclusive statement about what is or is not being supported by the security team. Is it IIS, or anything where Windows is involved? I'm seeing allusions to both of these in the previous comments.
    • If possible, if anyone has experience/expertise and can share, I'd like to see the documentation updated for "Best practices when running Drupal on Windows using Docker" or something along those lines. If my situation ever gets beyond inquiries and I have to figure it out, I'd be glad to contribute to such documentation, but I hope I'm not the first to even think about trying this.
    • Overall, I think I am more concerned about community support rather than security support, if that distinction makes sense. I know Windows is an edge case, and I'm unconcerned if the DST needs to exclude IIS and the Windows OS from security support. In a Docker-based scenario, chances are, any security issue would be more related to the architecture within Docker rather than the host anyway. What I would like to see is sharing of best practices by those who have had to support this use case. And to avoid a blanket "We don't support Windows" statement, but a more nuanced, "If you must use Windows, here is an approach that works."

    Thanks to the DST, core maintainers, and everyone else who has put so much thought into this. I'm writing this to share my limited experience with this scenario. If there is more I can do to help, let me know.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Also want to add an issue type to the discussion. What about php warnings? Usually I push back on those tickets because someone just puts an isset() or is empty() to mask an issue. But if thatโ€™s the fix does it need a test?

    Also agree on most of the suggestions here but would request some post be made in slack #contribute, #bugsmash, and #needs-review-queue-initiative when a final decision is made

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom pstewart

    Hi all, I'm one of the maintainers of the the sqlsrv module https://www.drupal.org/project/sqlsrv โ†’ and from what I can see this change shouldn't change anything with respect to that module for sites using SQL Server either as the primary database or as an auxilliary database. I suspect most users of the module are hosting in a Linux env and just using SQL Server for database (which is my own situation), however I'm guessing we're more likely to have above average Windows + ISS users versus the rest of the community, so I've posted in the #sql-server Slack channel to publicise this RFC.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    What I was thinking of when I wrote 'is a specific regression test needed' is we have in some cases written one-off tests that are very specifically written for 'is this bug that we used to have not there any more', and are not 'does this system work as it's intended to', and that over time makes the overall test suite harder to maintain, and reduces our test vs. coverage ratio (i.e. we've have several cases of multiple tests testing more or less exactly the same thing, added years apart in different issues). But I think the rewording in #105 is plenty here and covers that situation well enough.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    I have been watching this issue for a long time and never commented because I am very on the fence about it. On the one hand I see that we would move faster and fix more bugs if we committed some fixes without tests, but on the other I am worried about introducing regressions later on. But after sitting on it for a while I think the proposal here is a good compromise to both those arguments.

    I also agree with #100/102 that update tests are one of the most painful things to write, and concur that if the update hook is trivial then we should not require explicit testing; we do have test coverage that ensures that all updates are at least successful, and we have test coverage for e.g. the config/state APIs, so there seems little value in ensuring that an update hook can correctly make a minor change via those APIs. I further agree with #103 that CSS fixes and some accessibility fixes really can't be tested.

    Overall, the suggestion in the IS that the majority of the questions must be answered yes is a good failsafe. However:

    1. Is the fix is easy to verify by manual testing?
    2. Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc.
    3. Is the fix achieved without adding new, untested, code paths?
    4. Is an explicit 'regression' test needed?
    5. Is it easy for someone who did not work on the original bug report to add the test coverage in a followup issue?
    6. Does the issue expose a general lack of test coverage for the specific subsystem? If so, is it better to add generic test coverage for that subsystem in a separate issue?
    7. If this fix is committed with test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed?

    Question 4 stands out to me here. It's either the wrong way round - Is no explicit regression test needed? Yes means we can proceed without a test - or perhaps even it can be removed as a duplicate of question 7?

    The first part of question 6 also suffers from a similar problem, and I think "with" should be "without" in question 7.

    Suggested rewording where, to me, primarily "yes" answers point towards not requiring test coverage:

    1. Is the fix is easy to verify by manual testing?
    2. Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc.
    3. Is the fix achieved without adding new, untested, code paths?
    4. If this fix is committed without test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed?
    5. Is it easy for someone who did not work on the original bug report to add the test coverage in a followup issue?
    6. If the issue exposes a general lack of test coverage for the specific subsystem, is it better to add generic test coverage for that subsystem in a separate issue?

    I moved the final regression question higher as to me that is more important than the latter two questions.

    I also agree with #104 in that we could use "Needs no tests" when a decision has been made after answering the questions, or "Needs tests" if we decide the opposite, and also in conjunction with "Needs followup" if we decide to push test coverage to a separate issue (which should be opened and the tag removed before the bug fix is committed).

  • 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 dww

    P.s. weโ€™ve already got the โ€œNeeds testsโ€ tag. Do we want to introduce something like โ€œNeeds no testsโ€ to indicate weโ€™ve considered it and itโ€™s not worth it, undesirable, can be punted to follow-up, etc?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States dww

    +1 to #100 and #102.

    Here's another to consider: what tests could / should we add for ๐Ÿ› Active toolbar tray has weak affordance and fails WCAG color criteria Needs work ? That issue has already been so painful, I don't think I could handle another 2 years for the tests. ๐Ÿ˜ฌ I'm not really sure what we'd be testing. It's a major accessibility bug with an entirely CSS fix. Are we supposed to have automated coverage for the accessibility of our CSS (beyond the lint job, which is already passing)?

  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand DanielVeza Brisbane, AU

    I have a couple of issues that I think are worth raising in regards to this:

    โœจ Prevent auto-adding new fields to LB layouts Needs review is in the exact same situation that @acbramley raised in #100. The new functionality is tested, the only part that is missing a test is the update hook, which is simply setting a new boolean config value to TRUE.

    ๐Ÿ› Contextual links can be used to drag and drop Layout Blocks. Needs review is a one line fix on a 5+ year old issue. Testing the dragging Layout Builder blocks appears to be an existing issue because the existing tests around this simulate the element moving in the DOM rather than performing an actual drag.

    Testing this particular issue requires us to be able to click and hold on an element. We don't have this in core and I can't think of a way to do this in a test without writing code that I feel like may be prone to random failures.

    Considering this is a very old issue with a one liner fix that can be easily tested manually I think it may be a good candidate for this.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    I realise in comment #1 I said solid -1 but that was before the criteria were established.

    So +1 from me from a framework manager perspective.

    Leaving the tag so @alexpott and @effulgentsia can also opine

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    I'm wondering if we can also document when an update path test is needed?

    Take ๐Ÿ› Reduce the number of field blocks created for entities (possibly to zero) Needs review for example - a critical bug that greatly affects performance of large Drupal installs.

    The update hook is trivial - it sets a boolean value in a new config.

    Writing an update path test for that is not trivial - even for someone extremely familiar with writing tests, and someone that has written upgrade path tests in the past (like me).

    This one in particular is harder because layout_builder needs to be installed on top of the generic db dump file before we run updates, I spent ~60 minutes this morning getting this test working, and it would've been MUCH longer if I hadn't had multiple resources to help me (one being the existing MenuLinksetSettingsUpdateTest to copy from, and the layout-builder.php stub file that I stumbled upon to do the module install for me.

    We then run that update hook on every single MR going forward, all to test that a one liner runs correctly? Something that any number of contributors can look at and visually confirm it's doing the right thing.

  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone New Zealand

    The parent issue is in core and that doesn't really makes sense since this is in contrib. Moving that parent to related to help retain the history.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

    I have not encountered ISS yet in my professional carreer. However I did work on a few projects which used MS Azure and must say the process was not fun at all. Besides that performance was very poor and it took a lot of time and effort to get it to run somewhat well without slapping a huge amount of resources on the server. Azure itself also still has to my knowledge it's issues (DB level), but that's off topic here.

    In my opinion it perfectly makes sense to only allow linux environments. With the more modern approaches of Docker images and Kubernetes clusters I really don't see any problems with that. I think if this decision lands, this would even push "Windows only" clients into a more open source street, which is a positive side effect ๐Ÿ˜‰

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom scott_euser

    From database perspective I have had clients using Azure SQL as researchers are often working there. No issues using contrib https://www.drupal.org/project/sqlsrv โ†’ though and I asked platform.sh just a couple weeks back and they were happy to update driver support in php to 8.3 (was previously 8.1) so for those hitting this issue worried about connecting to such a database hopefully that helps.

    From a development environment perspective: as mentioned earlier WSL2 is a good option for Windows users. If you come across this issue and are worried if you can continue using Windows I would encourage you to try out https://ddev.readthedocs.io/en/latest/users/install/ddev-installation/#_... - I have had colleagues successfully use it fine across multiple big projects and Randy has a good test suite set up to make sure it continues to be supported there (https://ddev.com/blog/ddev-local-automated-testing/).

Production build https://api.contrib.social 0.61.6-2-g546bc20