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

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

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States smustgrave

I think this looks good! And good timing

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Seems all feedback has been addressed. There's 1 open thread though @longwave mind taking a look?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Geez those counts went way down!

Reviewing the change the early return seems straight forward and makes sense.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Appears to need a rebase.

Will this be a replacement for the performance checks where it assets the count between 2 values.

πŸ‡ΊπŸ‡Έ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

Verified test coverage here https://git.drupalcode.org/issue/drupal-3347042/-/jobs/884570

Thanks for taking a look @larowlan!

Believe all feedback has been addressed

πŸ‡ΊπŸ‡ΈUnited States smustgrave

@quietone can you elaborate more? As mentioned in #106 the function is deprecated. The parameter you mentioned is being added not removed so don't think it needs to be deprecated.

@rcodina just to make sure when this function is removed the feature you're working will still work correct?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Would this count as an API change? I feel yes but not familiar with the issue to say. But should be added to issue summary.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Does seem issue in #36 should address that comment. Restoring RTBC after reoll

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Just following up on this one if there are next steps

πŸ‡ΊπŸ‡ΈUnited States smustgrave

From #224 thanks for answering that!

Appears all feedback has been addressed

Unfortunately only original person or committers can close those threads :(

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Issue summary appears to be missing a few sections

Added the missing sections but left TBD on sections for someone more familiar with the issue to speak on.

Also can we hide some of the MRs to just have the intended for 11.x branch. Can be backported later depending when this lands.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

If a different solution is going to be used it should be reflected in the issue summary. #11 doesn't mention what the new solution is.

A separate MR should be opened and in the issue summary have something like

Option A

Option B

So discussion can be had about best approach.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So the proposed solution needs to include what is actually being proposed.

Fact this change didn't break anything says we don't have test coverage for this exception. Should see if we can add a quick one.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Thanks! Think I'm going to mark it so we can have it for 10.3 (not sure the cut off). But also see it's passing all database tests.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

In gitlab it looks perfect.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

using that plugin this is what I see

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Believe this one is ready again,

Can see the test coverage here https://git.drupalcode.org/issue/drupal-3043330/-/jobs/875234

Ran the same tests from #111

Setup a standard install locally with layout builder installed
Applied the MR without issue
Ran the database update, which ran fine.

Went to /admin/config/content/layout-builder and verify the checkbox was checked
Went into my Article content type which is using layout builder.
All the user fields (for example) appear to be there just as they were before. I don't have layout builder enabled for users

Unchecking the setting at /admin/config/content/layout-builder
Went back into my Article content type
No user fields appeared now (Yay!)

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Issue summary appears to be incomplete/missing some sections.

Also tagging for a test case to check the checkboxes under the scenario provided.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Feedback appears to be addressed around the annotation

Searched for @SearchPlugin and all 4 instances have been updated.

πŸ‡ΊπŸ‡Έ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 smustgrave

@dineshkumarbollu this was tagged for novice for new drupal users. Ticket wasn't a day old and looking at your post history you have over 27 pages of posts. Please avoid novice tagged issues for our newer users.

What's done is done though.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Spoke with @larowlan and we can push the test fixes to a follow up.

Opened πŸ“Œ Update layout builder tests and set new config Active

πŸ‡ΊπŸ‡Έ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 States smustgrave

Manually testing can confirm the issue is fixed. And don’t think it’s something we have to worry about being reverted accidentally.

Thanks for trying the tests!

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Let’s just remove the tests and mark it unless you can think of any other example in core that could simulate this

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So tried the test-only feature and locally but the test is passing without the fix.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Thanks for carrying this forward.

Left a comment on the MR but post_update hooks also requires test coverage. Should be a few examples in the views module that may help.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Read both change record and release notes and seem straight forward, so removing those tags

Trying my best to review MR

Applied locally and still seem to be able to run tests in phpstorm. Deprecation link work.

Think this is good. Should it be tagged 10.3 highlight?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So I asked @catch and doesn't see this as an issue even though book is deprecated.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

This appears to be updating the code. With that kind of change think issue summary should be complete.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Thanks for reporting,

Will need a failing test to show the issue.

MR target should be 11.x as the current development branch.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Seems this is postponed on πŸ› Sequences of mappings cannot merge keys from a subsequent definition of mapping keys Needs work

Also left a comment in [#15456258] about breaking that up.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Asked in #needs-review-queue-initative but since book is deprecated we have postponed all issues for it. What do you think about breaking this apart? Open a new ticket for adding the new constraint and postpone this to have book update in contrib

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Believe feedback has been addressed

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Wonder if IS could be updated to included proposed solution. Not sure if this counts as API change?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Believe all feedback has been addressed around MR but don't really follow the issue summary, specifically the proposed solution section. Will leave in review if anyone else wants to pick up.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Appears to be 1 more thread open from @Wim Leers around core/modules/filter/src/FilterFormatListBuilder.php

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Right but the div and em tags with those classes wrap around

{% if site_slogan %}

{{ site_slogan }}

{% endif %}
{{ 'Menu'|t }}

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Thanks! Reviewing this like I did Forum and believe everything has been addressed.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Believe the description contains enough detail. Wouldn't want to add much more personally.

needs a clean rebase with 11.x

Dumb question but is that different from a regular rebase?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Not the best with schema but wonder if the update needs to be in views.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Sure but should be documented

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Seems like a straight forward improvement and makes sense to me (I think)

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Could a CR be added?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

@Prem Suthar just FYI this was tagged for novice, meaning for new users to Drupal contribution, and based on your post history believe you could work on non novice issues please.

Believe the goal was just to update getComponentNames function.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Fyi did mention in #ux https://drupal.slack.com/archives/C1AFW2ZPD/p1708440649188029 here and it seems will require it to be brought up.

@rkoller made some good points

i think it might make sense to discuss the issue in a group context. i see a few aspects worth having a discussion. for one, for sighted users the used visual pattern might be more prominent or even use a warning admin notice or an info admin notice. but cross checked the user interface standards but there is no remark about that case: https://www.drupal.org/docs/develop/user-interface-standards β†’ but also from an accessibility perspective i wonder if the current proposed solution might go unnoticed for screenreader users? perhaps adding a detail in a visually hidden span to the page title might be an option? therefore i think it β€œmight” make sense to discuss it in a group context.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

I'm not 100% sure either but if that doesn't work I can just put the manual testing results and maybe that's enough

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So wasn't 100% sure how to best review this one

And of course gitlab is freezing up but one nitpicky thing I saw was

public function compare( could these new functions have a return typehint

As far as using the code I applied the patch for about an hour. Creating views with multiple relationships, created content and media, used layout builder and block layout. Just trying to make sure I touched as much as I could and nothing appeared to break.

I checked the CR and it reads well, the examples were extremely helpful so thank you for that!

For me I'm a +1 for giving this a shot but will leave in review for a bit longer for more eyes.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Will let you make the call on deprecation tests. Deprecation for sure. But tests for something that didn't work may be overkill then.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

If that solves this issue too this should probably be closed as a duplicate.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

I think Finalize makes sense too. I updated the CR and issue summary to reflect that.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Took the screenshots from #14 to the issue summary.

Believe feedback has been addressed,

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Know tour is being deprecated but as the maintainer for when it moves to core 100% want this included :)

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Seems like a feature that will need test coverage. Fact nothing broke shows we don't have it.

Deprecate Html::escapeCdataElement().

Still needed.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Left some comments on MR about the CR link.

But checked that the replacements were done. Probably good to self RTBC @mstrelan after the link updates.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

This will need an issue summary using standard issue template, most sections appear to be missing

Will need a failing test case to show the issue.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Believe this needs to use proper deprecation.

Question is do we still want to deprecate in D10?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Need to remove change to the script. Can see other related issues for guidance

πŸ‡ΊπŸ‡ΈUnited States smustgrave

so how come this one is postponed?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So what was the solution for the problem seen in #36? Or still being determined?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

As a UI change the proposed solution should have before/after screenshots in the issue summary (where its says TBD)

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Issue summary appears to be missing proposed solution section.

Also wonder if there's been a backtrace to see what's calling this with a string. In case we covering up a larger issue.

πŸ‡ΊπŸ‡ΈUnited States smustgrave
<div class="usa-logo site-logo" id="logo">
  <em class="usa-logo__text">

So not sure what's to be done. Everything with the logo is wrapped in this. So to add the other stuff back I think I would end up with what it is now.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Applied the suggestions by @Wim Leers.

Will this need an upgrade path though to remove roles key from existing config?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Doing searches for $value instanceof \Traversable and instanceof \Countable; and checking if they're also checking is_array, believe all have been addressed.

Left a comment but I would +1 for deprecating that one function since it does 1 thing now.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

MR is showing as unmergable. The spell-check has been fixed a week or two ago, branch needs to be rebased with latest 11.x

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So this does work with the twig block of alerts and patterns but when using with views it doesn't work due to {% if meta_items_tags %}
always returning true.

Pushed up a fix for that but still doesn't work great in views because of how views returns the string.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Thanks @Spokje!

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Should this postponed then?

πŸ‡ΊπŸ‡ΈUnited States smustgrave

How do we feel about merging this one and do a beta release for 3.0.x branch? See if it gets people interested.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Title and issue summary mention just layout_discovery.module but the patches appear to be entire drupal so tagging for those to be updated.

Also recommended to use MRs

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Left some small comments.

Will want sub maintainer sign off before merging.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

We should go back to commit https://git.drupalcode.org/project/drupal/-/merge_requests/6443/diffs?co... rest appears to be out of scope of the issue.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

See that all @HelpSection have been replaced and tests appear to be passing.

Applying locally I can still use help module just fine

+1 from me but will leave in review for additional eyes.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Go for it. Haven't checked the follow up tag need either.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

Sound like a broken record haha. Change looks good don't see any issue and nothing broke.

πŸ‡ΊπŸ‡ΈUnited States smustgrave

So reading remaining tasks

Update the misused words from the comments, particularly "unpreloaded", "uninstallations", "unstripped", and "untranslatable".

Has that been done?

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