Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update DocBlockHelper.php allow different date and time classes #948

Merged
merged 6 commits into from Nov 1, 2023

Conversation

markusramsak
Copy link
Contributor

@markusramsak markusramsak commented Oct 4, 2023

since CakePHP 5.0 the overwritten DateTime or Date class is not used anymore for annotation.
I can't get Checks to pass, but I think it is clear what I want to achieve. It worked in the previous major version.

@markstory markstory added this to the 3.x milestone Oct 4, 2023
Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have a test for this. You could have the date/time types mapped to the PHP DateTime classes.

@markusramsak
Copy link
Contributor Author

sadly I am not able to write tests because of lack of knowledge

@dereuromark
Copy link
Member

@markusramsak
The test docs should give you some insights
It is actually straight forward

You checkout your fork of this repo standalone, checkout your branch here, run

composer update
vendor/bin/phpunit YourTestFileClass.php

and adjust it with an additional test...() method that you want to test until it is green based
on your changes

@dereuromark
Copy link
Member

Also note that PHPStan still has an issue with the code it seems:

Instanceof between mixed and Cake\Chronos\Chronos will always evaluate to false.

@markusramsak
Copy link
Contributor Author

I don't know how to solve it. It was changed for CakePHP 5 from @LordSimal but it is a regression because now it doesn't work anymore with overwritten DateTime oder Date class. I don't know why this change was made in the first place ...

@dereuromark
Copy link
Member

Ah ok, so this fail is unrelated to your PR and already in the main branch - then we can ignore this one for now :)

@markusramsak
Copy link
Contributor Author

markusramsak commented Oct 20, 2023

I know I am missing the tests but the changes allow using custom Time/Date/DateTime classes which are used for annotations. Without this using only CakePHP default classes are used for annotation of entities which is regression to CakePHP 4.x and related bake version.
The change in cakephp/cakephp#17370 is related to this change.

@markusramsak
Copy link
Contributor Author

markusramsak commented Oct 20, 2023

How do I write a Test to confirm that my suggested code works? I want that Date, Time and DateTime classes could be overwritten (like before). How do I test this? It seems like a static validation to the Cake-Classes in the tests in DocBlockHelperTest.php at the moment ...

@LordSimal
Copy link
Contributor

This was removed in #929 because I thought this wasn't necessary anymore but clearly I was wrong 😅

@markusramsak
Copy link
Contributor Author

Please could help me someone to get the tests working for this pull request so this change can be merged. I cannot update my Entity-Docs anymore until this change is merged. This change is actually nothing new - it was like that before for CakePHP 4.x

@LordSimal LordSimal merged commit 18486ac into cakephp:3.x Nov 1, 2023
7 of 8 checks passed
@LordSimal
Copy link
Contributor

We can take care of a testcase later. I'll just do a new release to add this feature back

@markusramsak
Copy link
Contributor Author

We can take care of a testcase later. I'll just do a new release to add this feature back

thanks for your help

@markusramsak markusramsak deleted the patch-1 branch November 1, 2023 10:04
@LordSimal
Copy link
Contributor

Sorry for taking so long to get this back in

@markusramsak
Copy link
Contributor Author

Sorry for taking so long to get this back in

We are all doing this in our free time and have other obligations. I completely understand.
By the way I am from Austria too. I am from Kärnten / Klopeiner See.

@dereuromark
Copy link
Member

@LordSimal Do we also need a IdeHelper update for this?

@LordSimal
Copy link
Contributor

will have to check later. My initial problem was just the fact, that date columns were mapped to the Time class which shouldn't happen with this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants