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

FIX Cast possibly null link to string #1029

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Dec 8, 2022

Issue https://github.com/silverstripeltd/product-issues/issues/642

Fixes

ERROR [Emergency]: Uncaught TypeError: SilverStripe\Control\Director::absoluteURL(): Argument #1 ($url) must be of type string, null given, called in /home/runner/work/silverstripe-graphql/silverstripe-graphql/vendor/dnadesign/silverstripe-elemental/src/Models/BaseElement.php on line 940
https://github.com/silverstripe/silverstripe-graphql/actions/runs/3645001687/jobs/6154761063#step:10:1226

Exception happens during dev/build - api change on Director::absoluteUrl() means null is no longer accepted

@GuySartorelli
Copy link
Member

Still failing ci?

@emteknetnz emteknetnz force-pushed the pulls/5/broken-builds branch from f7cc9f2 to 4cf4027 Compare December 8, 2022 21:14
@GuySartorelli
Copy link
Member

Was that API change documented? I can't find it in the changelog.

@emteknetnz
Copy link
Member Author

emteknetnz commented Dec 8, 2022

Yes changed - https://github.com/silverstripe/silverstripe-framework/pull/10594/files#diff-983f05f9a107b9bf66a1ed4230e524be82118dd71f4a795529f8794b762c8b47R408

Got changed because signature was strongly typed due to a deprecated param

Not documented - plan is to do all the API changes in one big go - https://github.com/silverstripeltd/product-issues/issues/627

@GuySartorelli
Copy link
Member

Okay. I wasn't aware of that (I thought that was only going to be for the deleted API) so it's probably worth mentioning that either in standup or in slack so the whole team is across that approach. But that's fine.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

See silverstripe/silverstripe-cms#2806 (comment) for a discussion on this approach. Strongly typing the return type of the given methods is out of scope for CMS 5.

@GuySartorelli GuySartorelli merged commit 50f6318 into silverstripe:5 Dec 13, 2022
@GuySartorelli GuySartorelli deleted the pulls/5/broken-builds branch December 13, 2022 03:49
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.

2 participants