Problem/Motivation

This is a follow-up to #3241299-20: Properly return FALSE for invalid strings in DateTimePlus::checkArray() so that the correct exception is thrown. Drupal\Component\Datetime\DateTimePlus::checkArray() is a static validation function used for checking that date parts are valid. It is only called by DateTimePlus::createFromArray(). @xjm suggested that ::checkArray() should be made protected in the next major version.

Proposed resolution

Make DateTimePlus::checkArray() a protected method.

Remaining tasks

User interface changes

Introduced terminology

API changes

DateTimePlus::checkArray() will be a protected method.

Data model changes

Release notes snippet

Issue fork drupal-3566186

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dcam created an issue. See original summary.

dcam’s picture

dcam commented
Issue tags: +Novice

This seems like it's probably a good Novice issue.

priyansu18’s picture

priyansu18 commented
Assigned: Unassigned » priyansu18

Working on it.

priyansu18 opened merge request !14255

adwivedi008’s picture

adwivedi008 commented
Assigned: priyansu18 » Unassigned
Status: Active » Needs review

Hello priyansu18,
Reviewed the MR, and it seems fine to me

Moving the issue to needs review, let's get some more reviews, and then we can promote to RTBC

priyansu18’s picture

priyansu18 commented

Hi @adwivedi008, after the fix it was creating problem in test files. So, I have fixed that too. Now it's ready to review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Definitely believe this could use a CR, not sure any BC concerns.

priyansu18’s picture

priyansu18 commented
Status: Needs work » Needs review

Thanks for the feedback. I don’t see any backward compatibility impact here since the changes are limited to fixes.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

MR appears to need a rebase.

CR still needed.

alok_work23’s picture

alok_work23 commented

Hii,
I am interested in contributing and would like to help with this issue.
@priyansu18 - Are you still working on this? If you need help with the rebase or change record, I would be happy to assist.
If you're no longer able to work on this, I could take it over and complete the remaining tasks.

Thanks!

priyansu18’s picture

priyansu18 commented

I'm little bit busy in a project, need some time to check it. In the meantime @alok_work23 if you really want to work on it, you can, but pls be careful.

priyansu18’s picture

priyansu18 commented

If any issue comes up, please leave it as is, I’ll check it shortly.

avinash.jha made their first commit to this issue’s fork.

avinash.jha’s picture

avinash.jha commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

appears to be a bad rebase. There's a lot of changes in the MR that probably shouldn't be there, sorry moving to NW.

Also#10 still applies

Thanks all

jan.fetalvero made their first commit to this issue’s fork.

jan.fetalvero’s picture

jan.fetalvero commented

Rebased against current main and updated the issue fork. Change record has been drafted: https://www.drupal.org/node/3577926

jan.fetalvero’s picture

jan.fetalvero commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Needs work
Issue tags: -Needs change record

Thank you to all the new contributors who have worked on this issue! We appreciate your contributions to Drupal!

I have added a comment and code suggestion to the merge request and am setting the status to Needs Work for a change to be made. An unnecessary variable assignment was added to the merge request that needs to be removed.

I updated the change record and changed the examples. Before/after examples in change records aren't intended to show what was changed in Drupal. They're meant to help people who are coming to the record looking for answers about why their code is broken. They need information about what steps they should take to fix their code. So the before/after examples should reflect how to fix implementations.

sujal kshatri made their first commit to this issue’s fork.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

Thank you, @sujal_31. My feedback was addressed. These changes are pretty simple, so I don't think there's much else to say about them.

For the record, I didn't find any uses of the method in contrib. See https://search.tresbien.tech/search?q=DateTimePlus%3A%3AcheckArray.

  • catch committed 27baa881 on main 
    task: #3566186 Make DateTimePlus::checkArray() protected

By: dcam
By:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !14255

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.