πŸ‡³πŸ‡ΏNew Zealand @quietone

New Zealand
Account created on 30 April 2013, almost 11 years ago
#

Merge Requests

More

Recent comments

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand
πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

All the changes to the MR in the last two weeks were improvements, such as wording, adding links, and grammar. None of the changed altered the intent or workflow of the new 'Project Update Working Group'.

Therefore, I am restoring the RTBC.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Adding the deprecation issue because that can only be completed once there is a stable release of statistics in contrib.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

@smustgrave, I explained the reason in #6. Also, in the meta it is step 4 which is before step 5, which is this issue..

πŸ‡³πŸ‡Ώ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.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Adding parent

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I rebased and tests are passing now

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Going for a better title. (I don't want to read 'Follow-up for #3324150: ' when I am digging through git history. I can find that out if I need to read this issue)

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments. I didn't find any unanswered questions.

I read the MR and left some suggestions. I also think input is needed from Usability on the new strings. Adding tag for a usability review.

I read the MR and I do suggest a change. When I read "Provide #multiple property for email render element with correct validation." I ask myself why we would provide an element with incorrect validation. I think it could be reworded. I know it is minor.

Setting to NW.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

There are failing tests. I am setting this to Needs work.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

This is failing spell check. Adding tag for a reroll.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Needs a rebase.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

This was failing spell check so I rebased.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Needs a reroll

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

This needs a rebase.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments. I don't see any unanswered questions.

This issue is about olivero, I am changing the component.

For those adding screenshots, it is best to include a link in the issue summary so that everyone working on the issue can find the latest screenshots.

I then read the MR and it is good to see a thorough tests, and for multiple themes. This now has a dataprovider to test one page for three themes. To me, that is not a good use of resources. The testing could be done in a loop in the original test thus keeping the setup to one functional test. Perhaps, we leave this as is and add a followup to convert this test class to one method that uses a data provider. Adding tag for a followup.

There is also work in the MR.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments. I didn't find any unanswered questions or other work to do.

Leaving at RTBC.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Thanks to everyone who worked on the child issues to make this happen!

I am adding credit to myself for the overall admin required to keep this on track.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ .

Ah, a 9 year old issue. I like seeing the older issues getting near completion.

I read the IS and the comments. I didn't find any unanswered questions. However the proposed resolution does not match the direction taken in #57. This is more that a refactor, it should include that an interface is added. And also a CR.

I made comments in the MR that need to be addressed.

Changing to NW

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

The MR is fine but I am concerned about the implication for deprecating as explained in #22. It seems like we could make our lives easier in the future if we avoided having to create a way to deprecate for according to each plugin type. Could we not add a 'deprecated' and a 'deprecation_message' property now? What are the implications of doing that now? I would like to understand the pros and cons of that.
Thanks

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I did not update the change record. Happy to do that if folks agree to the name change.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

@nod_, thanks for making this issue!

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I chatted with @kim.pepper who wasn't not opposed to changing UserSessionFinalizer to UserSessionFinalize. Therefore, I have made a commit for that. And also updated a comment.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments. I didn't find any unanswered questions.

I read the MR and made some comments. One of which is questioning the name of the service so I am setting to NR to settle that.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

There is a stable release of Forum in contrib but it was released 4 February 2022. That should be updated before this is committed.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Committed and pushed 6bea4525a1 to 11.x and 796cc26845 to 10.2.x. Thanks!

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand
πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand
πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Fix anchors

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Move Tracker to list of deprecated

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Discussed with catch and changing to Critical because this can make sites unusable.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand
πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

quietone β†’ created an issue.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand
πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

This is on Version 8.9.x which is End of Life. Moving to 11.x

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Removing πŸ“Œ Remove forum from themes Needs work from the Issue Summary. That will be done when Forum is removed.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

We should add the changes from πŸ“Œ Remove forum from themes Needs work and close that.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand
πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

This was discussed in #need-review-queue-initiative in Slack, https://drupal.slack.com/archives/C04CHUX484T/p1708034907274609.

The conclusion was that this is best done in Drupal 11 when removing the extension from core. This applies to Book, Forum and Tour.

@catch @quietone @wimleers (he/him) so not sure what’s needed to move forward on πŸ“Œ [11.x] Remove tour from themes Postponed any suggestion would be appreciated

Participants:

larowlan, smustgrave, quietone, catch, lauriii

Adding credit and changing parent.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments. I didn't find any unanswered questions.

The issue summary includes a list of API changes. If that is up to date this needs a change record.

Leaving at RTBC.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

The failed test was Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest::testLoginBlock which should be fixed by πŸ“Œ StandardPerformanceTest fails randomly on MySQL and consistently on Postgres RTBC .

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Assigning to myself to commit tomorrow morning.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments and the MR. I didn't find any unanswered questions or other work to do.

Leaving at RTBC.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Ah, this is my third visit here for triage. The recent changes are from my feedback and they have been addressed.

Leaving at RTBC.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comment, the MR and the CR. I didn't find any unanswered questions.

@james.williams, thank you for getting this issue to RTBC. I found your comments were always helpful and explained the changes and referred to the review that requested a change.

Starting with the CR, that needs to be updated, including the title. The CR should include before and after code samples. The title is to be a statement of the change without the 'should', this is a new requirement. Searching existing change records provides some examples that should help; https://www.drupal.org/node/3387233 β†’ , https://www.drupal.org/node/3014688 β†’ . I've added the tag for change record updates.

I read the CR, not a code review, and I didn't see anything that I thought should change.

I think all the core gates have been completed here.

Finally, let's improve the title of this issue so it says something about the fix, not the problem. Also, what is a 'proper' mechanism anyway? :-) Maybe, "Ensure path alias repository returns the correct fallback language'?

So, just a few things.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments, the MR and the CR.

I tend to think in lists, so here is what I found.

  • In #33 it is stated "by the time plugin alters run, derivers have already run". There is an issue to alter plugins early for migrate. I am adding that as a related issue.
  • In #73 there is an ask for a followup. I am not 100% sure it is still needed. But I am adding the tag so that others working on this issue can decide.
  • #74 says this should have been a stable blocker. I will consult with the other release managers on this.
  • In #83 a reminder was given to provide interdiffs. Thank you! Without the interdiffs it requires too much time to confirm that the requested changes were made so I did not. Fortunately, there are experienced contributors here and there is now an MR,
  • #95 suggests a different approach. Does this need discussion or a followup?
  • This is changing the UI, adding tag.
  • I read the MR (not a code review) and left comments that need to be addressed. This is changing schema and the remaining tasks asks for an update test. I do not see that test.
  • I read the CR and it does not mention the UI nor performance. It should tell the reader how to use the new settings, where to find it, what is the URL, etc. Usually with a UI change including a screenshot is helpful, though not a requirement here. If it is true that this effects performance then that should be explained in the change record.
  • I applied the MR and eventually figured out how to alter the setting. I found the description confusing. For me, the first two sentences are saying the same thing, that the field is always exposed. And is the 'impact' to reduce performance or increase it? I think that this needs input from Usability folks.
πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments and the MR. I didn't find any unanswered questions or other work to do.

Leaving at RTBC;

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments and the MR. I didn't find any unanswered questions.

I ran the inspection in PHPStorm which reported 6 problems in 4 files. Those are fixed here. There are two more changes here, one for \Drupal\Tests\views\Functional\Plugin\PagerTest::testNoLimit and the other in \Drupal\Tests\ComposerIntegrationTest::testComposerLockHash. How were these last two discovered?

I used the grep string from #8 which reports 138 matches in core. There may be more that can be changed but that should not prevent this improvement. We can also do a 'round 2'.

Leaving at RTBC;

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments. I didn't find any unanswered questions.

Leaving at RTBC;

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ .

Ah, this is one where I made the MR. The IS had duplicate sections, I have removed those.

Leaving at RTBC;

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments.

The only question here is whether a test is needed. By current policy a test is needed but I understand the point made in #2. I'll leave this at RTBC and let another committer express an opinion.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments and the MR. I didn't find any unanswered questions.

Thanks to @smustgrave for commenting on the remaining work here and getting the Issue Summary completed.

Leaving at RTBC;

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments for this issue and the followup and the MR for this issue. I don't see any unanswered questions.

The proposed resolution is not correct. I have updated it.

What I did find that needs work is the follow up. The issue summary over there is too brief. If I had not read this issue first I would have no idea what is to be done in that issue. I will tag that issue for an issue summary update.

Leaving at RTBC

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I reviewed this issue because I was doing triage on the originating issue.

The issue summary here should be more helpful. It should explain what work is to be completed here. As it is, a contributor has to read the parent issue and the change there first. And hopefully, they will find comment #19 over there that links to the deprecation policy that explains part of the process.

Also, the standard template should be restore so that all sections are available. Those sections are helpful in many ways. It is the first thing someone a contributor see when deciding to work on an issue so it should be inviting and explain the work clearly. It is also how we track of what work has been completed and what still needs to be done. And, speaking for myself, it is too often that I have to restore the template so I can complete a review. I'd rather not have to do that.

Typically, I summarize the remaining work in the remaining tasks section but I will do so here.

  1. Restore the standard issue summary template β†’
  2. Complete the template, explaining clearly what work is to be done here. Add link to the correct section of the Deprecation Policy.
  3. Consider tagging as Novice.
πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

This is my second visit here for triage. I didn't find any unanswered questions or other work to do.

Leaving at RTBC;

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

What nice change.

I'm triaging RTBC issues β†’ . I read the IS, the comments and the MR. I didn't find any unanswered questions.

The comment in the MR was not wrapped correctly. I fixed that by rewording the comment. Setting to needs review for that to be checked.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments and the MR.

Not all the points in #33 have been addressed. See https://git.drupalcode.org/project/drupal/-/merge_requests/3986/diffs#no.... I went back and skimmed the comments. The problem is that the MR and the latest patch are not the same. The MR needs to be updated with the latest, correct changes.

This also needs a title change that is not simply a problem statement. Something like, "Allow primary & secondary navigation focus outline to work with long text"

I hid all files except this initial image and the latest before/after screenshots.

I restored the issue summary so that a Remaining tasks section was available. Please keep all sections of the issue summary template to help reviewers and everyone keep track of the work on an issue. Thanks.

So, just a few things to tidy up here.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments and the MR. I didn't find any unanswered questions or other work to do.

The diff no longer applies, setting to needs work. Also, I left some comments in the MR.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Checking the RTBC queue.

Ah, this is the issue where I commented about the same person making changes in response to comments in MR and them setting those comments to resolved. At the time I didn't realize there are limitations on who can resolve comments.

There have been some changes to the MR but mostly just keeping it up to date.

Leaving at RTBC.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments and the MR. I didn't find any unanswered questions.

\This is a nice wee fix and with a test!

However, setting to NW for the test to use the testing profile. This is because we have working to reduce the usages of our resources. I expect the tests in the content_translation module can help with this.

πŸ“Œ [meta] Convert remaining tests using Standard profile to use Testing profile instead Fixed

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments. I didn't find any unanswered questions or other work to do.

The diff does not apply, tagging for a rebase.

Also tagging for a title update so we can say what the fix is in the title instead of just 'changing' something.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments. I didn't find any unanswered questions or other work to do.

Leaving at RTBC;

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments. I didn't find any unanswered questions.

This is failing commit-code-checks, specifically spelling. Setting back to Needs work.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS and the comments. I didn't find any unanswered questions or other work to do.

@DishaKatariya, thank you for the testing! When reporting results it helps committer and any other reviewers when the testing results are available from the issue summary. I know there are not many comments in this issue but when there are many comments and many test results over time it does help to have a pointer to the latest results.

I applied the diff and confirmed that the navigation with the arrow keys works as one would expect.

Leaving at RTBC

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

The tests have passed and they were minor changes so I am leaving this at RTBC.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

And the title of the CR could be more specific about where the operators method is used. Perhaps this is better, "Classes using operators() when extending \Drupal\views\Plugin\views\filter\FilterPluginBase must implement FilterOperatorsInterface'?

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments and MR. I didn't find any unanswered questions or other work to do.

The change record does need to be updated. It should state that using the operator method without the interface will throw an exception in Drupal 11.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

πŸ“Œ Restrict access to empty top level administration pages for overview controller RTBC adds an @todo item to remove some fallback code once this issue is committed.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I'm triaging RTBC issues β†’ . I read the IS, the comments and the MR. I don't see any unanswered questions.

This is tagged for an issue summary update which was completed in #22, so removing tag.

Adding tag for a follow up for this comment, https://git.drupalcode.org/project/drupal/-/merge_requests/4608#note_242911

When reading the MR (not a review) I made suggestions for several comments. Since they are minor I decided to go back and apply them, tests are running now.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

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

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I think this is a good idea. What i notice right now is when I read $base I keep asking myself 'base what?' Can $base be more specific? Maybe $base_definitions?

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

Oh, I have more reading to do. πŸ“Œ [11.x] Remove tour from themes Postponed is now postponed. I will get back to this later today.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

@smustgrave, thanks for creating the MR as well as the contrib project. I recently added a link to the contrib project to the issue summary of the parent issue.

That is a wiki page and anyone can update it. Tour is already listed in the https://www.drupal.org/node/3223395#s-upcoming-deprecations β†’ section. Of course, that will have to change when this issue is committed. It will move to the section of already deprecated extenstions. But I am still considering how to indicated the core version when an extension was removed.

This is postponed on a stable release of the contrib project and πŸ“Œ [11.x] Remove tour from themes Postponed .

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I think we need to spell check core when "core/.cspell.json" changes, which isn't currently done.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

What about the concerns raised in #14?

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

This will fail with phpstan errors. One is about the deliberately broken plugin and another is for the missing '$deriver' property.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

@aayushmankotia, thank you for your interest in this issue.

Drupal is no longer using a patch workflow, all work is done in the merge request. See the top of this issue to find details about the merge request. The documentation on Drupal is in the process of being converted. I suggest you read Using GitLab to Contribute to Drupal β†’ . Thanks.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

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

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

@scott_euser asked that the status of this issue be change to 'closed won't fix'.

I have searched the issue and there is no commit here. I skimmed through the comments and do not see anything, like a documentation change, to suggest that this has been fixed. #36 closed this as outdated and the next comment changed the status to 'Fixed'. That was the mistake. If I know the issue where this was fixed there is a possibility that this could be closed as a duplicate and credit transferred. But, there is no reference to another issue so 'closed outdated' is the status to use.

Also, noting that there are comments suggesting the use of contrib projects, #57, #58, #59

I am restoring the status from #36

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

And I forgot to remove the tag.

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

I chatted with @benjifisher about the followups. Two already exist related to the first item in #12. There are πŸ› The test-only script for GitLab CI does not reliably use the right commit Needs review and πŸ› Test-only job fails with "couldn't find remote ref refs/heads/11.x" when 11.x branch does not exist in fork Needs review . I have added a new issue for the second point. πŸ› Let the test-only script for GitLab CI fail when it tries to access a "bad object" right commit Needs review

πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand
Production build https://api.contrib.social 0.61.6-2-g546bc20