πŸ‡ΊπŸ‡ΈUnited States @smustgrave

Account created on 30 June 2015, almost 9 years ago
  • Software Engineer at MobomoΒ 
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States smustgrave

smustgrave β†’ changed the visibility of the branch 3332473-refactor-claros-table--file-multiple-widget to hidden.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Appear to be in the home stretch with the last few remaining Congrats to everyone

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Tested my theory and I was wrong so restored to what it was. LGTM

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Super easy to review

the 10 items in the summary match the 10 removed in the MR
Checking the changes seems to be changing the key "testkey" which seems fine
Nothing stands out to me

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Appears to have test failures now.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

smustgrave β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

@Mithun S thanks for checking but screenshots were already uploaded in #7.

Thanks @ahsannazir for updating the summary, looks good to me that was the only holdup from before.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

#10 points still apply. Believe this is just turning #9 into an MR.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Should use the existing MR going to close this one

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Then I may be a -1 for it as it feel kinda awkward with the current placement

πŸ‡ΊπŸ‡ΈUnited States smustgrave

https://git.drupalcode.org/issue/drupal-3445425/-/jobs/1898662 shows it passing, ran locally it's passing

Not sure how much drupalCi is maintained but last option is for 10.1.x which doesn't apply here unfortunately.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Have not reviewed yet but issue summary appears to be incomplete could that be updated please

Can use https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... β†’ . for help if needed.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Do you have access to the tugboat side and can share the preview? just to make sure the config built correctly?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Not sure this one that can get tests or is large enough for them so will remove that tag, any disagrees please add back.

Will tag for an after screenshot to be added though

πŸ‡ΊπŸ‡ΈUnited States smustgrave

That CR is fantastic very nice!

Checking the MR believe the open threads were related to the previous solution which has since pivoted.

Issue summary appears to be inline with everything.

Code wise the change makes sense and I don't see any issue

https://git.drupalcode.org/issue/drupal-3449181/-/jobs/1832340 shows the test coverage.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Still not a huge fan of [0]. personally but it was used in the previous commit so would assume fine here.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

smustgrave β†’ changed the visibility of the branch 3259381-follow-up-for-jquery to hidden.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Hiding patches for clarity

Removing tests tag as coverage was added

1) Drupal\Tests\contact\FunctionalJavascript\ContactFileFieldTest::testFileFieldHasPrivateSchemeByDefault
Failed asserting that false is true.
/builds/issue/drupal-3336081/core/modules/contact/tests/src/FunctionalJavascript/ContactFileFieldTest.php:72
2) Drupal\Tests\contact\FunctionalJavascript\ContactFileFieldTest::testFileFieldHasPublicSchemeByDefaultWhenPrivateSchemeNotConfigured
Behat\Mink\Exception\ResponseTextException: The text "It is advised to store file uploads for contact forms as private files. You can configure this in settings.php" was not found anywhere in the text of the current page.
/builds/issue/drupal-3336081/vendor/behat/mink/src/WebAssert.php:907
/builds/issue/drupal-3336081/vendor/behat/mink/src/WebAssert.php:293
/builds/issue/drupal-3336081/core/tests/Drupal/Tests/WebAssert.php:975
/builds/issue/drupal-3336081/core/modules/contact/tests/src/FunctionalJavascript/ContactFileFieldTest.php:115
FAILURES!
Tests: 2, Assertions: 20, Failures: 2.

Applied some formatting changes to the hook and tests but appears @larowlan feedback from #7 has been addressed.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Looking at the issue summary the solution doesn't match the solution so should be documented why the solution fixes the problem.

Please no snippet of the code but the why

Also an after picture should be added the summary also.

Thanks.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Cleaning up MR and patches some. Could we add the theme setting?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

smustgrave β†’ changed the visibility of the branch 3163765-add-option-to to hidden.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Sorry for the delay so with the MR what is fixed for you?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Small super nitpicky change

But MR should be pointed to 11.x vs 11.0.x as 11.x is still the "main" branch.

I see the test-only feature passes when it should fail so tests seem to need some work.

As far as your comment on the MR I also could not find a spot and we don't have a filter sub-maintainer so think it's fine for now.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

@sokru thanks for hiding the old MRs, also good to hide all patches since those aren't needed anymore and the bot could pick them up.

Left some comments on the MR.

πŸ‡ΊπŸ‡ΈUnited States smustgrave
1) Drupal\FunctionalJavascriptTests\Theme\ClaroViewsBulkOperationsTest::testViewBulkOperationAlter
Behat\Mink\Exception\ElementNotFoundException: Button with id|name|label|value "Custom button" not found.
/builds/issue/drupal-3447611/core/tests/Drupal/Tests/WebAssert.php:158
/builds/issue/drupal-3447611/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroViewsBulkOperationsTest.php:124
FAILURES!
Tests: 2, Assertions: 33, Failures: 1.

Now seeing the failing test on the MR. Removing tag

Believe feedback has been addressed, code change looks fine.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

smustgrave β†’ changed the visibility of the branch 3447611-claro-theme-loses-test-only to hidden.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So it's possible this is a works as designed

Not disagreeing it's difficult to read, but if it's to change think we need make sure it's not a performance hit.

πŸ‡ΊπŸ‡ΈUnited States smustgrave
1) Drupal\Tests\field\Kernel\EntityReference\Views\ViewsSelectionLimitTest::testLimitedOutput
Failed asserting that actual size 20 matches expected size 10.
/builds/issue/drupal-2772523/core/modules/field/tests/src/Kernel/EntityReference/Views/ViewsSelectionLimitTest.php:84
FAILURES!
Tests: 1, Assertions: 8, Failures: 1.

Shows the test coverage so removing that tag.

Left some small comments on MR

Appears to be close!

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Thanks @kksandr for restoring what was removed in 54 and previous MR

πŸ‡ΊπŸ‡ΈUnited States smustgrave

smustgrave β†’ changed the visibility of the branch 2772523-entity-reference-views to hidden.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So asked @larowlan about this and he mentioned this change now introduces run-time code, while the code in HEAD is optimized by PHP when asserts are off (as they should be in production)

Not sure a path forward here

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Going to lean on this being small enough to not need test coverage so will mark.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Neat. Will definitely need a CR for the template addition.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Hiding patches for clarity

Previously was tagged for tests so moving to NW for that.

Thanks.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Didn't review but MR should be against 11.x vs 11.0.x

As the author should be able to update the target, probably will require a rebase.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Don't believe this has been addressed

should say what it means when this happens.

Also why would the parameter not be initialized?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Move seems straight forward and non disruptive since function names are same.

I drafted a simple CR to announce the new trait here https://www.drupal.org/node/3455776 β†’

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Should be an MR

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Samit why did you open a new MR? Looks identical to the existing one

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So the linked issue has been closed and haven't received a follow up if still a bug so going to close for now.

If still an issue please reopen.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

I'm still not able to replicate on 7.0.x. can the steps be written out vs in screenshots please.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Just tested on 7.0.x branch trying to follow the steps but not able to replicate the issue.

Will need additional steps to be written out. Please put them in list order vs sentence easier to follow thanks.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So unfortunately I don't believe there is anything we can on this side to fix the problem. Also verified that patch on core fixes the issue so left a comment to maybe help shake it loose but encourage others to get involved with it.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

I do believe this has been fixed. Following the steps

I checked "Rewrite the text displayed based on key"
And added All|Select... to the replacement

Worked as expected nothing in the logs

πŸ‡ΊπŸ‡ΈUnited States smustgrave

@feyp not sure I follow the suggestions? Do you want to add them to the MR?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

I just tested again and ajax is working for me. Can you provide additional details.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

I changed my tune, no one seems to be asking for this so went ahead and removed the API.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Finally got around to this one. Sorry again for the noise

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Lets see if that fixes it.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

I think it will as it is changed behavior automatically. Could be simple though with before/after screenshots maybe of the altered results?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

I was just testing Ajax recently on 7.0.x the current development branch and it was working so may need more detail

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Anyone test the related issue to see if that solves it?

This issue will need a summary update: missing exact steps and proposed solutions.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Or provide more detailed steps

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Are you able to test 7.0.x on D10?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

This was pretty close way back when so going to merge now in contrib.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

There's a README now and composer.json we could update both to include.

I add to the moduel page.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

There's a banner on the module page about it.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

May be out of scope for this module pending a major rewrite. But would maybe try https://www.drupal.org/project/entity_hierarchy β†’

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Will move to the current development branch if anyone ends up working on it.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Will say occasionally there are issues that target older branches. But know almost all have to land in 11.x first.

Also 11. is just the name because "main" couldn't be used.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So book has been removed from core so not sure the title makes sense anymore. Though I do plan to include this on Book as a good alternative

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So still not 100% how to test this one

I applied a script that @xjm was kind enough to share
php -r "include 'vendor/autoload.php'; \Drupal\Composer\Composer::setDrupalVersion('.', '10.whichever.whatever');"

And set to 10.5.0

I altered the variables SECURITY_COVERAGE_END_DATE_10_5 and SECURITY_COVERAGE_ENDING_WARN_DATE_10_5 to go back 1 year.

But with or without the patch the page on /admin/reports/updates remains the same

πŸ‡ΊπŸ‡ΈUnited States smustgrave

@xjm sorry I was referring to πŸ› Container compile crash when a service decorates a destructable service Needs work

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Actually had to revert as it broke the tests.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

If fixed in 11.x but not 10.3 then should find out if something was committed that maybe didn't get backported.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Does appear to be the standard and good canidate for 2.0.x

πŸ‡ΊπŸ‡ΈUnited States smustgrave

11.x is the current development branch so fixes need to land there first and be proven there also, just FYI

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Needs reroll for contrib only.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

This reproducible without another patch?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Currently this module does not have such a feature built in.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

I'm fine with this but will probably need to move config to a test module because this will break a lot.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So now that this has moved to contrib I'm not opposed to the vertical tabs. Feel placing in the middle would seem awkward, and menu links are over there which is somewhat similar. This still a valid request or something that couldn't be handled by custom code?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Seems to be a duplicate of https://www.drupal.org/project/book/issues/3103207 πŸ“Œ Create kernel test to Form\BookSettingsForm Needs work which has some work.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

This may count as not needing tests but think should least include a comment for the change.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Changed deprecation to 12 vs 11. But deprecation looks good.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Have not tested but issue summary appears to need some work

Previously tagged for tests which believe are still needed.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

MR should be against 11.x as the latest development branch

Feels like a change that needs some kind of test coverage also.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Seems like a good refactor.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Going to remark as tests are green and these seem like MRs that will always have conflicts so may be nice to get this chunk in. But testControllers appear reverted.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Tagging for steps as just install drupal isn't triggering it.

Will need to research why the value isn't there vs a check. Don't want to mask a larger issue.

πŸ› | Book | Breaks site
πŸ‡ΊπŸ‡ΈUnited States smustgrave

More information will be needed. Installing seems fine

Production build 0.69.0 2024