Re based the MR, moving this for NR. Kindly review.
I have addressed all the feedback, including updating the comment for the single-word condition to improve clarity. Please let me know if we need to assert the single word in the test for further clarification.
Moving this to NR , Kindly review.
AS #6 is not applying, Fixed conflict.
Rebased and fixed conflicts.Moving this back to NR.
Added the clearCaches() method to PerformanceTestTrait, allowing other performance tests, such as OpenTelemetryFrontPagePerformanceTest and OpenTelemetryNodePagePerformanceTest, to use it. After this change, the clearCaches() method was removed from these performance test classes.
Added follow up ticket for this : https://www.drupal.org/project/drupal/issues/3504566 📌 Move clearCaches() Method to PerformanceTestTrait for Reusability Across All Performance Tests Active
shalini_jha → created an issue.
@smustgrave Thank you so much for your detailed explanation!
The clearCaches() method needs to be removed from the TestTrait. Yes, I found several performance test classes that could benefit from this change, so you are absolutely right—we should create a follow-up ticket to update all performance test methods accordingly.
For now, I have added the clearCaches() method within the same test, following the pattern used in other performance tests.Pipeline is also green , so moving this back to review.
Re base this MR and fixed the conflict .
I have checked the MR, and the changes made in #11 were removed from the MR after the latest rebase. I can no longer find the comment-related changes in the MR. moving this back to NR.
@borisson_ Thank you for your review & feedback. i am looking into it.
I Have checked this MR and found the pipeline failure for the enabled rule. I have updated the test class accordingly, following the same approach as in the other files. After making these changes locally it is also passing, and the pipeline is also green. I am now moving this MR for review.
shalini_jha → made their first commit to this issue’s fork.
@quietone Thank you so much for the clear explanation! I have gone through the documentation, and it makes sense. I truly appreciate your guidance :)
I Have checked this existing MR , and based on the IS i have run the phpcs command and got the failure what pipeline is also showing for ClassComment. i have updated all the mentioned class comment with referencing the existing class comment , and again rerun the phpcs locally and it is passing same as pipeline is also passing .
So moving this for NR, please review and let me know if any of class comment needs to update.
shalini_jha → made their first commit to this issue’s fork.
@quietone You are correct. I have also checked the usage of ImageStyle::getReplacementID, and I could not find any.
I have tried to address the added feedback, but I have two points to discuss:
Single Quotes: If we use single quotes, I have reviewed the entire MR and found additional places where updates are needed. Should we update all occurrences for consistency?
Full Stop in Keys: I have reviewed the MR and noticed the same behaviour for adding a full stop in keys.
Please review and let me know. Thanks
shalini_jha → made their first commit to this issue’s fork.
I am a bit unsure about the last feedback. Could you please clarify if we need to remove this ?
moving this for NR to get the feedback.
Re based this MR with fixed conflicts in phpcs.xml.dist.
shalini_jha → made their first commit to this issue’s fork.
I am moving this for review. Please take a look and let me know if it makes sense, or if any additional tests need to be added?
Thank you @smustgrave for the review & feedback. the scenario i have mentioned is the existing test that is failing after the code change
I Have updated the issue summary based on the changes we have done so far. waiting for the confirmation of removal of uri_callback.
1 usage in contrib found http://grep.xnddx.ru/search?text=comment_uri&filename=
Removed this line as given url is not working
@oily I was just seeking confirmation and discussing it with you. No worries at all—let’s see what others think about this. I’m okay with their perspective, and perhaps I was mistaken. No problem, we can focus on another ticket instead of continuing the discussion here.
@oily I was referring to this ticket: # 3497796 📌 Add return type to hook_file_download implementations Active . The comment update happened after the issue reached RTBC, and that is not the scope of the ticket and unnecessary.
I just meant that since the issue is already in RTBC status, if any changes are needed, please feel free to add a comment. I also want to ensure the issue is completed properly, I am trying to complete the issue what i have started. That’s why I am making this request.
Ah yes i missed to this IS update tag , sorry for the inconvenience, i am working for this.
@smustgrave Can you please review the last commits? It seems these changes might not be necessary. I noticed that @oily has made similar changes in another ticket, which also seem unnecessary.
@oily, if you believe this change is required, please add a comment. I’m already working on this ticket, I will address.
Re base this MR and fixed the conflict . Also address the feedback mentioned #16. Regenerate Baseline. Pipeline is green so moving this back to NR. please review
shalini_jha → made their first commit to this issue’s fork.
Address the feedback on MR also rebase and fixed the conflicts, Moving this NR.Please review.
I have addressed all the feedback and made some improvements. I refactored the code to create a more generic method for the test. Additionally, I updated the comments, After re-running the tests, I verified that they are working correctly for both FileItem and ImageItem, handling both empty and custom file directories.
I am moving this to NR. Kindly review and let me know if any further updates are required.
Assigning this me for sometime , as i am working on it now to complete the feedback.
Thank you for the review @quietone, I will look into the feedback.
Based on the issue and its summary, it seems that this change is related to updating the return type. But last commit is updating the comments ?
Yes you are right , This is just a random failure , i have just rebase this MR.
I came across this issue through a referenced ticket. I searched for similar and found only TopBarPerformanceTest.php in the testTopBarPerformance method. The MR has already been created.checking for pipline failure .
Fixed conflict and re base this MR, re run the "grep "fileDownload\\\\.* has no return" core/.phpstan-baseline.php", and it is not showing any other list.So moving this back to NR.
shalini_jha → made their first commit to this issue’s fork.
I have addressed the feedback and made the necessary updates. The pipeline is green,
so I am moving this to 'Needs Review.' Kindly review.
shalini_jha → made their first commit to this issue’s fork.
Thank you for the feedback and reference. I've updated the code to use ::assertMetrics() as suggested and applied the same change to VocabularyAdminPerformanceTest. I also re-ran the test, and the query count is now updated to 12, so I have updated the test accordingly.
Moving this to Need review , Kindly review.
Fixed Bot reported error, so i am moving this NR instead of RTBC.
Re based & fixed conflicts , AS per #20 & #27 Moving this NR for answer.
Addressed all the feedback. Based on #75, I have checked the uri_callback: 'comment_uri' in other places. I found it in EntityInterface.php. Do we need to update this as well?
Based on #76 detail, I am moving this to Needs Review to get confirmation.
Re base & fixed conflicts. moving to NR.
Rebase without conflicts , so moving back to previous status.
Thank you for the review and feedback :)
I’ve addressed one of the points. Regarding the hook_update_N test coverage, I haven’t worked on this type of test before, so it is still pending. I’ll try to find a solution for it.
Rebased & fixed conflicts :)
Style lint is failing in this MR : https://git.drupalcode.org/issue/project_browser-3499390/-/jobs/4003797
Thank you for the confirmation. Since everything is addressed, I am moving this to Needs Review.
shalini_jha → made their first commit to this issue’s fork.
As Mentioned in #30 , i have fixed the pipeline failure by generating the baseline. pipeline is passing , so moving this again NR.
shalini_jha → made their first commit to this issue’s fork.
Rebased this and resolved conflicts. However, after running the command:
grep "_requirements\\\\.* has no return type specified" core/.phpstan-baseline.php
I noticed that two additional entries are still listed:
'message' => '#^Function install_check_requirements\\(\\) has no return type specified\\.$#',
'message' => '#^Function update_check_requirements\\(\\) has no return type specified\\.$#',I’m unsure if we also need to update these?
shalini_jha → made their first commit to this issue’s fork.
Rebased this without encountering any conflicts
I have updated the if condition to handle the scenario, and after testing, the test is now passing successfully. I am moving this for review. Please take a look and let me know if it makes sense to you.
I have rebased this MR and encountered multiple conflicts. I carefully resolved all conflicts; however, this still requires further review. Therefore, I am moving it back to the "Needs Review" stage instead of "RTBC." Kindly review.
shalini_jha → made their first commit to this issue’s fork.
Thank you for the review and feedback. Based on the suggestions provided, I tested the functionality after implementing the recommended changes, and it is now working as expected. There is no need for an array_merge after retrieving the arguments from _raw_variables. I also re-ran the tests, and they are passing successfully.
I have also drafted a CR → based on the work done and provided an explanation of the changes made. However, I’m unsure if anything else needs to be added. I kindly request you to review it and share your feedback. Thank you!
I Have able to replicate this pipeline failure locally and able to fix this. but i have added the approach for confirmation for fetching the latest revision.Moving this to review for some guidance.
Thank you for the review. You are right; I have updated the issue summary and title to reflect the changes we have made so far.
I have updated the security coverage icon title for both grid and list modes. For the project detail popup, it was already added. Moving this to review. Please review
Re base and fixed conflicts.
shalini_jha → changed the visibility of the branch 3492153-project-usage-icon to hidden.
Thank you for the review. I have added separate test coverage for this functionality and verified that it is working as expected. Moving this for review . Please review
Thank you for the review.
I have implemented the changes in the doGetUploadLocation method and reviewed all occurrences of this method. While we already addressed the issue in the FileItem class, I found that the ImageItem class was handling file paths in a similar way appending static slash, so I have applied the same fix there as well. To ensure the changes work as expected, I have added test coverage for scenarios involving a blank file directory in the ImageItem class.
Please review the changes and let me know if any further adjustments are needed.
Looking into separate test coverage
I previously reviewed this and confirmed that the "usage icon" is not included in the grid view; it is only present in the list view.
I have double-checked this to ensure accuracy, and I can confirm that this behaviour is same. The icon is display only in the list view and not in the grid view.
I’m moving this for review again. Please let me know if I’ve missed anything.
shalini_jha → made their first commit to this issue’s fork.
I have addressed the feedback and regenerated the PHPStan baseline. Moving this to NR for review. Kindly review.
Updated the CR according to the feedback based on the issue summary also. Moving this for NR.
let me know if anything else need to update.
i am working for CR Update
Rebase & fixed conflict,moving this NR.
I am working on it
@berdir, Thanks you for the Information :)
@berdir Thank you for the feedback , I have removed the uri_callback and tested the functionality. It is working as expected. However, I am a bit unsure if there is anything else that needs to be updated related to this.
Moving this for NR , Kindly review.
I have replicated the issue and reviewed the feedback on the MR, which makes sense. I have added the comments based on my findings. Additionally, I noticed that the assertion was passing every time, so I updated it and tested the change. Moving this to NR for confirmation or any further guidance.
shalini_jha → made their first commit to this issue’s fork.
Removed the comment on the return statement, as I believe it was not appropriate.
Kindly review.