Account created on 7 May 2021, about 4 years ago
  • Engineer-drupal backend at QED42 
#

Merge Requests

More

Recent comments

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

shalini_jha made their first commit to this issue’s fork.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

shalini_jha made their first commit to this issue’s fork.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

Thank you for the review and feedback. i am working on it.

🇮🇳India shalini_jha

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!

🇮🇳India shalini_jha

Rebased and fixed conflicts also regenerated the based line for fixing the pipeline failure. Moving this back to NR.Kindly review

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

shalini_jha made their first commit to this issue’s fork.

🇮🇳India shalini_jha

Hi, I have re-run the previously failed test and it has now passed. It appears to have been a random failure :)

🇮🇳India shalini_jha

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?

🇮🇳India shalini_jha

Addressed all the feedback , moving this NR, Kindly review.

🇮🇳India shalini_jha

Rebased without conflicts , so moving this previous status.

🇮🇳India shalini_jha

Re based the MR, moving this for NR. Kindly review.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

Rebased and fixed conflicts.Moving this back to NR.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

@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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

@borisson_ Thank you for your review & feedback. i am looking into it.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

@quietone Thank you so much for the clear explanation! I have gone through the documentation, and it makes sense. I truly appreciate your guidance :)

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

@quietone You are correct. I have also checked the usage of ImageStyle::getReplacementID, and I could not find any.

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

Re based this MR with fixed conflicts in phpcs.xml.dist.

🇮🇳India shalini_jha

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?

🇮🇳India shalini_jha

Thank you @smustgrave for the review & feedback. the scenario i have mentioned is the existing test that is failing after the code change

🇮🇳India shalini_jha

I Have updated the issue summary based on the changes we have done so far. waiting for the confirmation of removal of uri_callback.

🇮🇳India shalini_jha

1 usage in contrib found http://grep.xnddx.ru/search?text=comment_uri&filename=
Removed this line as given url is not working

🇮🇳India shalini_jha

@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.

🇮🇳India shalini_jha

@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.

🇮🇳India shalini_jha

Ah yes i missed to this IS update tag , sorry for the inconvenience, i am working for this.

🇮🇳India shalini_jha

@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.

🇮🇳India shalini_jha

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

🇮🇳India shalini_jha

Address the feedback on MR also rebase and fixed the conflicts, Moving this NR.Please review.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

Assigning this me for sometime , as i am working on it now to complete the feedback.

🇮🇳India shalini_jha

Thank you for the review @quietone, I will look into the feedback.

🇮🇳India shalini_jha

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 ?

🇮🇳India shalini_jha

Yes you are right , This is just a random failure , i have just rebase this MR.

🇮🇳India shalini_jha

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 .

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

I have addressed the feedback and made the necessary updates. The pipeline is green,
so I am moving this to 'Needs Review.' Kindly review.

🇮🇳India shalini_jha

shalini_jha made their first commit to this issue’s fork.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

Fixed Bot reported error, so i am moving this NR instead of RTBC.

🇮🇳India shalini_jha

Re based & fixed conflicts , AS per #20 & #27 Moving this NR for answer.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

Rebase without conflicts , so moving back to previous status.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

Thank you for the confirmation. Since everything is addressed, I am moving this to Needs Review.

🇮🇳India shalini_jha

As Mentioned in #30 , i have fixed the pipeline failure by generating the baseline. pipeline is passing , so moving this again NR.

🇮🇳India shalini_jha

shalini_jha made their first commit to this issue’s fork.

🇮🇳India shalini_jha

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?

🇮🇳India shalini_jha

Rebased this without encountering any conflicts

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

shalini_jha made their first commit to this issue’s fork.

🇮🇳India shalini_jha

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!

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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.

🇮🇳India shalini_jha

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

Production build 0.71.5 2024