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 undefined index error in CMS #2845

Merged

Conversation

NightJar
Copy link
Contributor

@NightJar NightJar commented Mar 28, 2023

With the CMS 4.12 update functionality was altered to utilise an Extension to obtain the CMS Edit link for a page, rather than having SiteTree do it internally. Unfortunately the default return case for extend (see Extensible) is an empty array. This leaves code potentially referencing an array offset that doesn't exist ([0]). PHP 8 is less forgiving than its predecessors on this kind of behaviour. We should check that the responses from extensions exist before trying to reference them.

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.

Thank you, this is great.
Just a small change.

code/Model/SiteTree.php Outdated Show resolved Hide resolved
// This method has to be implemented here to satisfy the CMSPreviewable interface.
// See the actual implementation in CMSEditLinkExtension.
return $this->extend('CMSEditLink')[0];
return $this->extend('CMSEditLink') ?? '';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return $this->extend('CMSEditLink') ?? '';
return $this->extend('CMSEditLink')[0] ?? '';

Still need to get the 0th result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops haha 😅

With the CMS 4.12 update functionality was altered to utilise an
Extension to obtain the CMS Edit link for a page, rather than having
SiteTree do it internally. Unfortunately the default return case for
`extend` (see Extensible) is an _empty_ array. This leave code
potentially referencing an array offset that doesn't exist ([0]). PHP 8
is less forgiving that it's predecessors on this kind of behaviour. We
should check that the responses from extensions exist before trying to
reference them.
@NightJar
Copy link
Contributor Author

Is there anything left to do here?
Can you rerun the behat task please? I feel like this failure is due to typical behat test flakeyness, not this change.

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.

Sorry, I got distracted and lost sight of this. Thank you for submitting it!

@GuySartorelli GuySartorelli merged commit 241fa3e into silverstripe:4.12 Mar 30, 2023
@NightJar
Copy link
Contributor Author

Thanks for restarting the automated test run, it's a relief to see it all pass :)

@NightJar NightJar deleted the pulls/4.12/undefined-index branch March 31, 2023 02:54
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

2 participants