🇺🇸United States @smustgrave

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

Merge Requests

More

Recent comments

🇺🇸United States smustgrave

Believe it’s a wrapper thing. Like all the other services call their respective interfaces

🇺🇸United States smustgrave

Ended up going with solution #2 and just tweaking it with a message so people know to fix their blocks.

🇺🇸United States smustgrave

Needs way more work and not entirely sure what a good BC way would be

🇺🇸United States smustgrave

Some reason the test-only is failing but not showing any output, so ran locally

Which shows the coverage.

Summary appears to be complete and no open threads

LGTM.

🇺🇸United States smustgrave

Wonder if the IS could get some love. Using the standard template please

Good to see the tests still pass but believe will still need test coverage

Thanks.

🇺🇸United States smustgrave

Issue summary appears incomplete, would recommend adding back the missing sections.

There are also multiple MRs when there should be 1 pointed to 11.x

Also not sure maintainers would want to move away from variables to hardcoded values

🇺🇸United States smustgrave

Since there hasn't been a follow up in 4+ months going to close out. If still an issue please reopen.

🇺🇸United States smustgrave

I'm a huge +1 for this. Mark has been super helpful cleaning, triaging, and reviewing issues around stable9. He also seems to have a strong desire to help and follow the current workflow of everything

Can you comment on the issue to confirm that you've read the https://git.drupalcode.org/project/governance/-/blob/main/drupal-core.md and that you understand and accept the responsibilities for the subsystem maintainer role?

I can't add you just something I've seen on other maintainer tickets.

🇺🇸United States smustgrave

Just using the bef test module and converted one of the fields to checkboxes + autosubmit.

🇺🇸United States smustgrave

Left comments on MR for consistency

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

🇺🇸United States smustgrave

Should we move back to NW for a new solution?

🇺🇸United States smustgrave

Thanks, yea personally think it's messy and think the current examples of it aren't good standards. So I'll leave in review for others to take a look.

🇺🇸United States smustgrave

Not 100% sure about the solution. Not sure if there is a function that can called to do some of this and more but does address the issue. Will let committer decide if its good enough.

🇺🇸United States smustgrave

Apologize for letting this one sit.

But does need a rebase again

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

🇺🇸United States smustgrave

Think we should have a simple test case, I prematurely RTBC in #15 and @nod_ in #16 how this broke things.

🇺🇸United States smustgrave

Since not reproducible in 7.0.x going to close out, if experiencing that there please please reopen.

🇺🇸United States smustgrave

Will leave for another, I can't reproduce. I've added checkboxes to a test form, enable autosubmit and it's working as expected.

🇺🇸United States smustgrave

you sure this in bef?

Also what is the real world scenario that you would do a URL like that?

🇺🇸United States smustgrave

Came up again but hasn't been much change since. Does seem like a task to me also though.

🇺🇸United States smustgrave

Since it's been 4+ months without an update, going to close out. If still an issue please provide an actual issue summary.

🇺🇸United States smustgrave

Rebase seems good and feedback appears to be addressed.

🇺🇸United States smustgrave

So I'm tempted to close this one out as there doesn't seem to be clear steps to reproduce.

🇺🇸United States smustgrave

Searched for * @Plugin( and all instances appear to be in a test. So seems pretty straight forward.

🇺🇸United States smustgrave

Have not reviewed

but issue summary appears to need some love

MR should point to 11.x vs 11.1.x and will need test coverage

Thanks!

🇺🇸United States smustgrave

Odd, definitely ran multiple times guess 4th time was the charm!

Test-only feature has already been ran and shows coverage for the change.

Don't see any open threads or remaining tasks, believe this is good.

🇺🇸United States smustgrave

Believe we should have test coverage for such a problem.

🇺🇸United States smustgrave

Closed the old MRs for. 10.3 as they were failing and probably missed the boat for them.

Rebased 11.x and still all green

All threads appear to be resolved, templated to tag 11.2 highlight but will let committer decide.

🇺🇸United States smustgrave

Closing as outdated as the interest seemed to be low, if anyone comes along and wants to pick it up please reopen :)

🇺🇸United States smustgrave

Feedback believe has been addressed.

🇺🇸United States smustgrave

Want to get a beta 7.1.x release out and include this if possible

🇺🇸United States smustgrave

Hiding patches.

I've ran the javascript tests 3 times but seem to fail consistently.

🇺🇸United States smustgrave

Not sure know what you mean need to run twice. With this change though shouldn't need to

🇺🇸United States smustgrave

Probably will need test coverage as well.

🇺🇸United States smustgrave

This came up as a daily bugsmash target.

From what I can tell the code is still the same. But I also wonder if this is a task or feature request vs a bug?

🇺🇸United States smustgrave

Since it's been over a year and still can't reproduce going to close out for now. If still an issue please reopen.

🇺🇸United States smustgrave

Appears to need a rebase.

Wonder if the follow up tag is still needed?

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

🇺🇸United States smustgrave

Believe close think one of the last pieces is to add a CR.

Example not super clear in core/modules/block_content/src/Plugin/Block/BlockContentBlock.php why

#[Autowire(service: 'block_content.uuid_lookup')]
Is needed but the others aren't, think a CR should clear that up.

🇺🇸United States smustgrave

Is there any performance measurement that may help with the review?

🇺🇸United States smustgrave

Won't this mean that entityFormAlter() won't be called?

🇺🇸United States smustgrave

Appears to need a rebase

@longwave what's a good way for testing this one?

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

🇺🇸United States smustgrave

Thanks for reporting!

Think we should get some test coverage around this.

Thanks!

🇺🇸United States smustgrave

Could the solution suggestion be flushed out a bit?

🇺🇸United States smustgrave

Why don't we do the kernel tests here and maybe push the functional to a follow up, since that may need some trial and error.

🇺🇸United States smustgrave

Nice! Seems straight forward

🇺🇸United States smustgrave

If that’s the case there is dead code throughout where similar code is used. So if it’s dead code should address all instances

🇺🇸United States smustgrave

Pipeline appears to have failures.

Probably will need test coverage

🇺🇸United States smustgrave

There was already an MR for 11.x should continue there

🇺🇸United States smustgrave

Should include test coverage with it

🇺🇸United States smustgrave

Appears to be missing test coverage

🇺🇸United States smustgrave

Thanks @mikelutz for that

Checking the MR appears all threads are resolved

However when I run the test-only feature it passes when it should fail so not sure if the bug is being covered.

🇺🇸United States smustgrave

Updating claro seems out of scope. Also on other views core ships we don't expose the operator.

For me this seems like a unique need so I may be a -1 for adding to core.

🇺🇸United States smustgrave

Seems like more info is needed to move it forward

🇺🇸United States smustgrave

I’ll leave for someone else then. But typically see that when tests are pushed to a follow up they usually don’t happen

🇺🇸United States smustgrave

2 small comments I missed first round, that's my fault.

🇺🇸United States smustgrave

Took another look and schema appears incomplete. There’s no mappings in it.

If it’s a new configuration will need an update hook for existing sites.

May need a change record as well

The test is there an existing test that could just be extended?

🇺🇸United States smustgrave

Unless nightwatch doesn’t work in the test only feature I’d expect the test to fail without the fix

🇺🇸United States smustgrave

With 6.0.x support low is this a problem in 7.0.x?

🇺🇸United States smustgrave

Since there hasn't been any IS or steps to reproduce in a few years going to close out. If still an issue please reopen.

Production build 0.71.5 2024