-
Notifications
You must be signed in to change notification settings - Fork 117
MNT Fix behat test #1406
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
MNT Fix behat test #1406
Conversation
GuySartorelli
left a comment
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.
It looks like this PR would be hiding a bug - we shouldn't need a CMSEditLink in order to add a non-inline-editable block to a DataObject.
|
Perhaps? I don't have a lot of context on this one, I just traced it back to this PR so I restored edit link functionality. Do you have context on this one? |
|
I don't have enough context to know what the underlying cause of the failure is, but I do know that we shouldn't need an edit link on the parent in order for the steps in this behat feature to pass. |
Not to add, to edit the block - failing test is https://github.com/silverstripe/silverstripe-elemental/blob/6/tests/Behat/features/add-block-element-to-data-object.feature#L26 which has It needs the CMSEditLink() here https://github.com/silverstripe/silverstripe-elemental/blob/6.1/src/Models/BaseElement.php#L877 Note that $page the parent DataObject, not specifcally a Page, and I think that If $link is null (which happens when we are missing that extension I added in this PR), then there's no logic to handle it .. which makes sense? Seems like you'd need a nested Call stack to get to that code is BaseElement::getElementCMSLink() |
|
Ahhhh yup that makes sense. In that case I think it should fail with a hard exception that has a clear message, instead of just trying to navigate to... somewhere? We should also ensure that the documentation calls our this requirement. |
|
Have created a new issue for the hard exception - behavioral change so needs to be in a new minor We still need these PRs to fix 6.1 CI |
GuySartorelli
left a comment
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.
LGTM
Issue #1405
Requires silverstripe/silverstripe-frameworktest#257 for behat to go green