Skip to content

Changes include to include_once in shortcode files.#8023

Merged
oskosk merged 1 commit intomasterfrom
fix/shortcode-includes
Oct 19, 2017
Merged

Changes include to include_once in shortcode files.#8023
oskosk merged 1 commit intomasterfrom
fix/shortcode-includes

Conversation

@zinigor
Copy link
Copy Markdown
Contributor

@zinigor zinigor commented Oct 19, 2017

This is needed since #7991 has landed in Jetpack. There is a particular edge case that illustrates the problem. After #7991 the widget would check if a shortcode is present, and would load it if it wasn't. If then the shortcode would try to include that file again, it would fatal. This could happen when you have the widgets module enabled, and shortcode modules disabled. The fatal would appear in the request when you're trying to enable the shortcodes module.

Changes proposed in this Pull Request:

  • Changes include to include_once for shortcode files.

Testing instructions:

  • Disable shortcodes module.
  • Enable widgets module.
  • Try enabling the shortcodes module again using the legacy modules page.
  • Observe the fatal error.
  • Try to do the same thing again with this PR, observe no fatal.

This is needed since #7991 has landed in Jetpack. There is a particular edge case that illustrates the problem. After #7991 the widget would check if a shortcode is present, and would load it if it wasn't. If then the shortcode would try to include that file again, it would fatal. This could happen when you have the widgets module enabled, and shortcode modules disabled. The fatal would appear in the request when you're trying to enable the shortcodes module.
@zinigor zinigor added [Feature] Shortcodes / Embeds [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Oct 19, 2017
@zinigor zinigor added this to the 5.5 milestone Oct 19, 2017
@zinigor zinigor requested a review from jeherve October 19, 2017 19:27
@zinigor zinigor requested a review from a team as a code owner October 19, 2017 19:27
Copy link
Copy Markdown
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

Works as expected, and fixed my env thanks! :shipit:

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 19, 2017
@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Oct 19, 2017

Merging because it makes sense.

@oskosk oskosk merged commit 9537298 into master Oct 19, 2017
@oskosk oskosk removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 19, 2017
@oskosk oskosk deleted the fix/shortcode-includes branch October 19, 2017 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Shortcodes / Embeds

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants