🇺🇸United States @smustgrave

So if it were a warning that would make sense?

smustgrave → created an issue.

smustgrave → created an issue.

Finally picked this pack and was able to knock out 4 more.

smustgrave → created an issue.

I just installed the module and it ran fine am I missing something?

This plugin already uses a setting form so expanding to an a "Alternative host path to test" wouldn't be terribly difficult is that you were proposing?

Actually not able to replicate this mind providing additional steps.

Hey sorry mind providing more concrete steps to reproduce? Changing the default language doesn't break anything for me.

Does https://git.drupalcode.org/project/security_review/-/merge_requests/86/d... solve this?

Actually maybe a bug?

       if ($writable_htaccess) {
          $findings[] = 'writable_htaccess';
          if ($result !== CheckResult::FAIL) {
            $result = CheckResult::WARN;
          }
        }

This actually is rendering as a failure

Not sure I follow what would the other command be?

Glad to hear about it please add to https://www.drupal.org/project/uswds_ecosystem →

3 subsystems and I never am needed for sub maintainer review. Benjifisher pinged me about this one. Assuming to look at the approach of using the onRemoval approach +1 for me, pretty inline with how we do other places.

Did take a lap at saving credit.

NR isn’t the correct status

Let me elaborate more. Not knocking the code snippet at all. But is it a valid scenario?

I could write faulty code that could break things and that would be on me not the system to handle.

If a valid scenario a test case should be added

LGTM

Okay why would you do that though?

I’ll try and find time. This module has been lacking in my attention that’s my fault

All good now!

Mstrelan may know. He’s been doing a ton of work around fixing those

#29 and #30 should be addressed believe before this is put back into review.

Also seems people are posting maybe the same or similar errors but issue summary hasn’t been updated

Also wouldn’t recommend patches file as fixes are in the MRs

Sorry for the delays thanks for clarity. Cleaned up the pipeline some

Going to close this one out. Think we are going to keep backboneJS in contrib but don't want to cause noise for others so opened 📌 Update backbone.js and move off of core Postponed

smustgrave → created an issue.

Since it had been a few weeks didn't know if @jatin812 was coming back so I added the test coverage.

Thanks all

Random javascript and functional test failures, re-running green

Running locally with 8.5.0RC3 1 test triggered 2 PHP deprecations:
With the MR 0

Change LGTM

Going on a limb and assuming new exception doesn't need a CR.

I'd have to see it live. Not opposed to changing my solution I just couldn't get the link to cover the card any other way.

Thanks for clarifying that!

Believe all feedback here has been addressed.

Updates to type hints in documentation are typically classified as tasks, not bug reports. This doesn't need tests or steps to reproduce.

That's not actually true wrong docs can be considered a bug.

Could we document in the summary why those not deprecated should remain?

Ran locally on 8.5.0RC03

Got 1 test triggered 1 PHP deprecation:

1) /var/www/html/core/modules/content_moderation/src/ViewsData.php:73
Using null as an array offset is deprecated, use an empty string instead

With the MR got 0

Change LGTM

Why is the typehint needed in core/modules/config/tests/config_test/src/Entity/ConfigTest.php ?

Feedback appears to be addressed.

Left additional comments in the MR.

Other one is RTBC what more would need to be done here?

LGTM

@oily isn't not needed to update summaries all the time for sections that are NA

Pipeline still fails.

Since there's been no follow up and a feature request going to close out. There hasn't appeared to be interest in this but it can always be re-opened.

Thanks!

Reads more like a feature request and is this something we want to do?

Going to mark as this does fix the issue. Will tag but weird case I'm not sure needs framework manager sign off or not.

Have not reviewed but the issue summary appears incomplete

This addressed the deprecations in the tests mentioned.

appears to have kernel and functional test failures.

Seems straight forward enough.

Can we update the summary some. Proposed solution means just catching the exception and moving on. But seems we are doing more then that.

Also several pipeline issues

Left comments.

Locally on 8.5.0RC3

1 test triggered 1 PHP deprecation:
With the MR I got 0

Change LGTM

Thanks for reporting but will need test coverage

Just in case we don’t end up needing deprecation may want to update the title too

Assuming #18 meant to RTBC this one?

Possible to expand the test coverage to check the exception message too.

Looking at the MR vs the CR think we need to deprecate _update_cron_notify before we can remove it.

I asked about this to one of the view maintainers and summary will need to be updated for a modern use case. Appears it was useful for facets at one point but no longer the case. New features typically need to hit a 80% usefulness for others and this right now does not feel like that.

Will leave open for others to update or respond but may be closed in 3 months.

That same random javascript error, re-running is green

Exception: Deprecated function: Using null as an array offset is deprecated, use an empty string instead
Drupal\Core\FileTransfer\Form\FileTransferAuthorizeForm->setConnectionSettingsDefaults()() (Line: 312)

All green.

Also this file is marked deprecated to be removed in 12 so this fix doesn't break anything and isn't long for this world. LGTM

This came up as a daily BSI target.

Looking at the proposed code change and the code is still the same.

Cleaned up the summary some but may also need test coverage for this.

Thanks!

myself for 🐛 \Drupal\Core\Entity\Query\QueryBase never goes out of scope Postponed: needs info

Since there's been no follow up in 4 months going to close out. If still an issue in D11 please re-open

Thanks all!

@kartagis able to test on D11 site?

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Thank you for reporting this problem. We rely on issue reports like this one to resolve bugs and improve Drupal core.

Since there has been no activity here for over 8 years we are asking if this problem persists on a currently supported version of Drupal. To help, add a comment explaining if the problem still occurs or not. Any extra detail you can provide can help others who experienced this.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!

Appears to have caused several test failures.

Pretty straight forward.

It's not getting passed phpstan, may need to update the baseline.

Was hard to do a search for ->add( but did some spot checking and all instances calling Symfony console add I believe have been replaced.

Seems like a good deprecation.

Tested locally with 8.5.0RC3

This one was weird as locally it actually failed

1) Drupal\Tests\content_moderation\Functional\WorkspaceContentModerationIntegrationTest::testModerationInWorkspace
Exception: Deprecated function: Using null as an array offset is deprecated, use an empty string instead
Drupal\content_moderation\ModerationInformation->shouldModerateEntitiesOfBundle()() (Line: 82)

But with the MR it's all green

LGTM

Thanks for reporting.

MRs need to be against 11.x as the development branch.

Would be good to get a test case showing this bug

Tests look good.

Still seems to have 1 kernel failure

Stable9Library Override (Drupal\KernelTests\Core\Theme\Stable9LibraryOverride)
✘ Stable 9 library overrides
┐
├ Drupal\Core\Extension\Exception\UnknownExtensionException: The theme stark does not exist or is not installed.

No test failures so seems like a good cleanup.

Huh?

Will see if the core issue lands. It’s a feature request I’m not 100% convinced it’ll be accepted

Rock on!

See 📌 Attempt to break part tour.js Active

smustgrave → created an issue.

Will admit I tried breaking apart the files but just ended up breaking everything, couldn't get around shepherd is now a .mjs when in core it was an older version that was just js.

Ran locally on 8.5.0RC3

ViewsModerationStateSortTest gave me 2 tests triggered 1 PHP deprecation:
With the MR 0

Going to mark but should we add coverage for when it would ['']['base']? Don't want to hold up the issue if a bad question

Short and straight to the point CR nice!

Tests coverage is for the deprecation https://git.drupalcode.org/issue/drupal-3361177/-/jobs/7282504

Code wise pretty straight forward, seems like a good trigger and makes sense.

Manually testing and does address the issue described.

LGTM!

Is this not a duplicate of 🐛 An empty views pager offset field can cause a PHP type error to be triggered. Needs work

Kernel test failures seem related to the change, example Drupal\Tests\image\Kernel\ImageEffects "system module is not installed."

Locally using 8.5.0RC3 did not work so ran in the gitlab runner

Ran FieldKernelTest got 5 tests triggered 1 PHP deprecation:
Ran FieldDropbuttonTest::testDropbuttonMarkupShouldNotLeakBetweenRows got 1 test triggered 1 PHP deprecation:
Ran FieldSelfTokensTest got 1 test triggered 1 PHP deprecation
Ran FieldWebTest::testTextRendering got 1 test triggered 1 PHP deprecation:
Ran FieldWebTest::testAlterUrl got 1 test triggered 1 PHP deprecation:

Locally I'm still getting 1 deprecation,

1) /builds/issue/drupal-3557459/core/lib/Drupal/Core/Config/Schema/Mapping.php:185
Using null as an array offset is deprecated, use an empty string instead

Is this in scope? Example for FieldKernelTest https://git.drupalcode.org/issue/drupal-3557459/-/jobs/7282445

Locally on 8.5.0RC3

Running EntityReferenceFormatterTest gave me 3 tests triggered 1 PHP deprecation:
Applying the MR I got 0

Way to make me feel like a noob didn't know you can pass multiple variables to check :(

LGTM!

Seems straight forward, weird ValidateHostnameTest needed to be updated but seems fine.

lets do it.

Locally using 8.5.0RC3

Running testResolveInternalIncludePath got 4 tests triggered 1 PHP deprecation:
With the MR got 0

Already see we got the commented out typehints awesome!

Going to move to NW for the tests, but think we need them here? We know these kind of deprecations work

Thank you!

Wow you must be lucky you got random javascript failures in all 3 threads. Re-running and it's all green

Seems like a straight forward fix and leaning on @phenaproxima knowledge for this one.

LGTM

Ran locally on ddev 8.5.0RC3
6 tests triggered 1 PHP deprecation:
With the fix 0 deprecations

LGTM

Since there's been no follow up in a few years and a feature request going to close out. It can always be re-opened

Thanks all

Wanted to bump this 1 more if something we still need/want to do.

Wanted to bump 1 more time if still valid.

D7 is no longer supported and know the version checking that happens now has been pretty reliable so believe this can be closed. If anyone disagrees feel free to re-open

Thanks all

Believe to still be a valid task.

wanted to bump 1 more time before closing. Not heard of this being an issue so can't fully speak on it.

I actually think this is actually worth doing. If you have even 3 text fields all that default text can be a lot of scrolling.