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

API Added extension points to ShortcodeParser #6302

Merged
merged 1 commit into from
Nov 9, 2016

Conversation

jonom
Copy link
Contributor

@jonom jonom commented Nov 7, 2016

Allows us to solve problems like sheadawson/silverstripe-shortcodable#8

@helpfulrobot
Copy link

@jonom, thanks for your PR! By analyzing the blame information on this pull request, I identified @sminnee, @dhensby and @chillu to be potential reviewers

@jonom
Copy link
Contributor Author

jonom commented Nov 7, 2016

To complement this change, might be good to change a lot of the protected stuff to public so that extensions can make use of it. In particular there's no way at the moment to get info on registered shortcodes, data is stored in protected $shortcodes = array() with no getter. You can see if a shortcode is registered with registered($shortcode) but can't look up the class and method responsible for rendering it.

Would there be any drawback to opening this stuff up?

@dhensby
Copy link
Contributor

dhensby commented Nov 8, 2016

#5535 and #5987 are related.

I think adding some public getters makes sense and can go into the 3 branch too

@jonom jonom force-pushed the shortcode-parse-extension-points branch from 835cc8f to ffd9938 Compare November 8, 2016 19:24
@jonom
Copy link
Contributor Author

jonom commented Nov 8, 2016

@dhensby have added a getter for registered shortcodes which seems like the most useful thing to expose for now and doesn't mess with visibility of existing methods.

@dhensby dhensby merged commit cfbfe14 into silverstripe:3 Nov 9, 2016
@jonom jonom deleted the shortcode-parse-extension-points branch November 9, 2016 16:34
@jonom
Copy link
Contributor Author

jonom commented Nov 9, 2016

Thanks Dan

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.

3 participants