-
Notifications
You must be signed in to change notification settings - Fork 79
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
NEW Use embed/embed v4 #1254
NEW Use embed/embed v4 #1254
Conversation
61ee159
to
17f18a0
Compare
|
||
use Exception; | ||
|
||
class InvalidUrlException extends Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we creating this exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented in reply below
230969f
to
56480fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InvalidUrlException
sound too generic for something that is very oEmbed
specific. At the very least, there should be a comment on the class to explain its purpose.
I'm wondering if it wouldn't make more sense to have this exception in framework instead?
56480fc
to
4821b46
Compare
Have renamed to Note there's something of an unavoidable (semver breaking?) API change in that the Exception thrown has changed. It's unavoidable because the old Exception came from embed/embed v3, which no longer exists. |
4821b46
to
e049bc7
Compare
Build is failing because it's still trying to install an older version of frameworktest. I took the liberty to bump it up |
3f079bb
to
3546bd7
Compare
Actually, the framewortest version got fixed in #1259 already. I removed my extra commit, merged up the other fixed and rebased. |
Issue silverstripe/silverstripe-framework#10243
Requires silverstripe/silverstripe-framework#10244