Addressed the mentioned feedback on the MR. Also tested the failing test — it appears to be a random failure, so I re-ran the pipeline. All pipelines are now green. Moving this back to NR. Kindly review.
shalini_jha → made their first commit to this issue’s fork.
I have gone through the feedback on the MR and addressed the requested changes.I also re-ran the tests locally, and they are passing.
Moving this back to Needs Review for further feedback.kindly review.
shalini_jha → made their first commit to this issue’s fork.
I have checked the issue and applied fixes based on the MR feedback. I updated the code to cast to bool directly, as this handles both true and false cases. There is no need for a ternary operator in this situation. I have reverified the test, and it is passing as expected.
Additionally, I encountered linting failures for the existing code. I have addressed these issues, and the code now passes the linting checks. I am moving this back to Needs Review. Kindly review.
shalini_jha → made their first commit to this issue’s fork.
I have rebased this MR and resolved the merge conflicts. During the rebase, I noticed that some new behavior was introduced, so to ensure everything continues to work as expected, I re-ran the range test locally.
While debugging, I found that the test was failing in the third scenario where the datetime value falls within the defined range. Due to the newly introduced behavior Here ✨ Datetime fields should utilise #required_error Needs review , the form state now includes additional errors such as datetime_required_error and datetime_no_required_error.
To address this, I updated the third scenario’s assertion to only check that range_datetime_element is not present in the form state errors, as this is the specific error we are handling in this case in testRangeValidate() test method. With this change, the test now passes as expected.
Moving this back to Needs Review.Kindly review.
shalini_jha → made their first commit to this issue’s fork.
Thank you for the feedback , yes {@inheritDoc} is there so no need to add the deprecation here , i have checked other deprecated method also for same with {@inheritDoc}. i have updated here also.
Thank you for your feedback!
I’ve addressed all the points as suggested, and rerun the pipeline — it's now passing.
I’ve also updated the issue summary to reflect the changes made based on the MR feedback.
Please have a look and let me know if anything further needs to be adjusted.
Thank you for the review and feedback. i am working on it.
I’ve deprecated the getReplacementID() method after confirming it isn’t used in core or contrib, and I followed a similar approach to other deprecated methods. Moving this NR , Could you please review and let me know if anything needs to be updated or improved?
Thank you for the guidance!
Rebased and fixed conflicts also regenerated the based line for fixing the pipeline failure. Moving this back to NR.Kindly review
Thanks for the feedback!
Based on your suggestion, I haven’t modified the doGetUploadLocation() method. Instead, I added a trailing slash check directly in the same function where the destination path is being built for both FileItem and ImageItem. This way, we avoid changing the method’s return value and reduce the risk of breaking other usages.
Could you please review and let me know if this approach works, or if further changes are needed?
I have also update the IS based on my current approach for more clarity. pipeline is green so moving this for your review.
Kindly review, Thanks
Re based and fixed conflicts. pipeline is green so moving back to NR.
shalini_jha → made their first commit to this issue’s fork.
I reviewed the pipeline failure and found that it was caused by the cspell issue and same checked for MR feedback ✨ Missing documentation for loadModelsForm() Active , and now the pipeline failure is fixed after following the feedback. Moving this NR for further feedback.
shalini_jha → made their first commit to this issue’s fork.
Hi, I have re-run the previously failed test and it has now passed. It appears to have been a random failure :)
I have followed the same steps outlined in the IS for the 'AI DeepChat Chatbot' and was able to replicate the same error when no AI Assistant is configured. I believe we may also need to review and update this block accordingly?
Addressed all the feedback , moving this NR, Kindly review.
Rebased without conflicts , so moving this previous status.
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