new calendar endpoints #831
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, I'm still a bit brain dead from being off for 2 weeks and trying to get back in the flow of things. I think the |
Preview changes with PlaygroundYou can preview the least recent changes for PR#831 by following one of the links below: |
|
Hello @mauteri , I've updated the PR description with more relevant info and fixed the simple errors. The remaining CS failures and Unit test failures are kind of intentional, to remind us what needs attention. I will now not be able to add some helping review comments, but open it up for Reviews anyway. |
|
I'm going to start adding some review-comments for you now @mauteri. Would be cool, if you find the time (or someone else) to go trough all of this me...melange. |
There was a problem hiding this comment.
@mauteri I added 16 comments to help navigate to the most problematic parts.
| * Class constructor. | ||
| * | ||
| * Initializes the `Endpoint_Template` object by setting the slug, callback, and | ||
| * plugin template directory. The parent constructor initializes the slug and callback, | ||
| * while this constructor adds the plugin template default. | ||
| * | ||
| * @since 1.0.0 | ||
| * | ||
| * @param string $slug The slug used to identify the endpoint in the URL. | ||
| * @param callable $callback The callback function to retrieve file name and path of the endpoint template. | ||
| * @param string $plugin_template_dir The directory path for the plugin templates. | ||
| */ | ||
| public function __construct( string $slug, callable $callback, string $plugin_template_dir = '' ) { | ||
| parent::__construct( $slug, $callback ); | ||
| $this->plugin_template_dir = ( ! empty( $plugin_template_dir ) ) ? $plugin_template_dir : sprintf( | ||
| '%s/includes/templates/endpoints', | ||
| GATHERPRESS_CORE_PATH | ||
| ); | ||
| } |
There was a problem hiding this comment.
This method may need some better docs, why the 3rd param is needed or why it may be useful to extenders.
| /** | ||
| * Locate a template in the theme or child theme. | ||
| * | ||
| * @todo Maybe better put in the Utility class? | ||
| * | ||
| * @param string $file_name The name of the template file. | ||
| * @return string|false The path to the theme template or false if not found. | ||
| */ | ||
| protected function get_template_from_theme( string $file_name ) { | ||
|
|
||
| // locate_template() doesn't cares, | ||
| // but locate_block_template() needs this to be an array. | ||
| $templates = array( $file_name ); | ||
|
|
||
| // First, search for PHP templates, which block themes can also use. | ||
| $template = locate_template( $templates ); | ||
|
|
||
| // Pass the result into the block template locator and let it figure | ||
| // out whether block templates are supported and this template exists. | ||
| $template = locate_block_template( | ||
| $template, | ||
| pathinfo( $file_name, PATHINFO_FILENAME ), // Name of the file without extension. | ||
| $templates | ||
| ); | ||
|
|
||
| return ( is_string( $template ) && ! empty( $template ) ) ? $template : false; | ||
| } |
There was a problem hiding this comment.
Maybe better put in the Utility class?
| /** | ||
| * Build the full path to the plugin's template file. | ||
| * | ||
| * @todo Maybe better put in the Utility class? | ||
| * | ||
| * @param string $file_name The name of the template file. | ||
| * @param string $dir_path The directory path where the template is stored. | ||
| * @return string|false The full path to the template file or false if file not exists. | ||
| */ | ||
| protected function get_template_from_plugin( string $file_name, string $dir_path ): string { | ||
| // Remove prefix to keep file-names simple, | ||
| // for templates of core GatherPress. | ||
| if ( $this->plugin_template_dir === $dir_path ) { | ||
| $file_name = Utility::unprefix_key( $file_name ); | ||
| } | ||
|
|
||
| $template = trailingslashit( $dir_path ) . $file_name; | ||
| return file_exists( $template ) ? $template : false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe better put in the Utility class?
There was a problem hiding this comment.
This class, as well as the three other classes, that extend Endpoint are probably the best start to reduce clutter.
I can feel that this can and should be optimized, but wasn't able to do so.
There was a problem hiding this comment.
This class, as well as the three other classes, that extend Endpoint are probably the best start to reduce clutter.
I can feel that this can and should be optimized, but wasn't able to do so.
| /** | ||
| * Creates a flag option to indicate that rewrite rules need to be flushed. | ||
| * | ||
| * This method checks if the generated rewrite rules already exist in the DB, | ||
| * and if its rewrite URL matches the recently generated rewrite URL. | ||
| * If any of this checks fail, the option to flush the rewrite rules will be set. | ||
| * | ||
| * This method DOES NO checks if the 'gatherpress_flush_rewrite_rules_flag' option | ||
| * exists. It just adds the option and sets it to true. This flag | ||
| * is being used to determine when rewrite rules should be flushed. | ||
| * | ||
| * @since 1.0.0 | ||
| * | ||
| * @param string $reg_ex_pattern The regular expression pattern for matching the custom endpoint URL structure. | ||
| * @param string $rewrite_url The URL structure for handling matched requests via query vars. | ||
| * @return void | ||
| */ | ||
| private function maybe_flush_rewrite_rules( string $reg_ex_pattern, string $rewrite_url ): void { | ||
| $rules = get_option( 'rewrite_rules' ); | ||
|
|
||
| if ( ! isset( $rules[ $reg_ex_pattern ] ) || $rules[ $reg_ex_pattern ] !== $rewrite_url ) { | ||
| // Event_Setup->maybe_create_flush_rewrite_rules_flag // @todo maybe make this a public method ?! | ||
| // @see https://github.com/GatherPress/gatherpress/blob/3d91f2bcb30b5d02ebf459cd5a42d4f43bc05ea5/includes/core/classes/class-settings.php#L760C1-L761C63 . | ||
| add_option( 'gatherpress_flush_rewrite_rules_flag', true ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method could be simplified if Event_Setup->maybe_create_flush_rewrite_rules_flag() could maybe become a public method ?! Which in itself could be simplified, like I described in #880
| /** | ||
| * Checks if the given endpoint type is an instance of the specified class. | ||
| * | ||
| * This method verifies whether the provided `$type` is an instance of the `$entity` | ||
| * class. It first checks if the `$entity` exists in the defined list of valid endpoint | ||
| * classes by calling `is_in_class()`. If the entity is valid, it further checks if the | ||
| * `$type` is an instance of that class. | ||
| * | ||
| * @since 1.0.0 | ||
| * | ||
| * @param Endpoint_Type $type The endpoint type object to check. | ||
| * @param string $entity The class name of the entity to check against (e.g., 'Endpoint_Redirect' or 'Endpoint_Template'). | ||
| * @return bool True if the `$type` is an instance of the `$entity` class, false otherwise. | ||
| */ | ||
| private static function is_of_class( Endpoint_Type $type, string $entity ): bool { | ||
| return self::is_in_class( $entity ) && $type instanceof $entity; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the given entity is a valid endpoint class in the current namespace. | ||
| * | ||
| * This method verifies whether the provided `$entity` exists in the predefined list | ||
| * of valid endpoint classes within the current namespace. It helps ensure that only | ||
| * valid classes (like `Endpoint_Redirect` or `Endpoint_Template`) are used when | ||
| * checking endpoint types. | ||
| * | ||
| * @since 1.0.0 | ||
| * | ||
| * @param string $entity The class name of the entity to check (e.g., 'Endpoint_Redirect' or 'Endpoint_Template'). | ||
| * @return bool True if the `$entity` is a valid endpoint class, false otherwise. | ||
| */ | ||
| private static function is_in_class( string $entity ): bool { | ||
| return in_array( | ||
| $entity, | ||
| array( | ||
| __NAMESPACE__ . '\Endpoint_Redirect', | ||
| __NAMESPACE__ . '\Endpoint_Template', | ||
| ), | ||
| true | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This two methods also look a lot like as they were better placed in a more common place.
| return; | ||
| } | ||
|
|
||
| // @todo "add_filter('feed_content_type')" here, if the subscribe-able feed need something different than text/cal. |
There was a problem hiding this comment.
Will be removed.
| // @todo "/feed/ical" could be enabled as alias of "/event/feed/ical", | ||
| // and called with "get_feed_link( self::ICAL_SLUG )". | ||
| $alternate_links[] = array( | ||
| 'url' => get_post_type_archive_feed_link( 'gatherpress_event', self::ICAL_SLUG ), | ||
| 'attr' => sprintf( | ||
| $args['feedtitle'], | ||
| $args['blogtitle'], | ||
| $args['separator'] | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Should GatherPress establish a route like example.org/feed/ical as an alias for example.org/event/feed/ical, which could be nice, but also a duplicated-content issue?
| /** | ||
| * @author Stephen Harris (@stephenharris) | ||
| * @source https://github.com/stephenharris/Event-Organiser/blob/develop/includes/event-organiser-utility-functions.php#L1663 | ||
| * | ||
| * Fold text as per [iCal specifications](http://www.ietf.org/rfc/rfc2445.txt) | ||
| * | ||
| * Lines of text SHOULD NOT be longer than 75 octets, excluding the line | ||
| * break. Long content lines SHOULD be split into a multiple line | ||
| * representations using a line "folding" technique. That is, a long | ||
| * line can be split between any two characters by inserting a CRLF | ||
| * immediately followed by a single linear white space character (i.e., | ||
| * SPACE, US-ASCII decimal 32 or HTAB, US-ASCII decimal 9). Any sequence | ||
| * of CRLF followed immediately by a single linear white space character | ||
| * is ignored (i.e., removed) when processing the content type. | ||
| * | ||
| * @ignore | ||
| * @since 2.7 | ||
| * @param string $text The string to be escaped. | ||
| * @return string The escaped string. | ||
| */ | ||
| private static function eventorganiser_fold_ical_text( string $text ): string { | ||
|
|
||
| $text_arr = array(); | ||
|
|
||
| $lines = ceil( mb_strlen( $text ) / 75 ); | ||
|
|
||
| for ( $i = 0; $i < $lines; $i++ ) { | ||
| $text_arr[ $i ] = mb_substr( $text, $i * 75, 75 ); | ||
| } | ||
|
|
||
| return join( "\r\n ", $text_arr ); | ||
| } |
There was a problem hiding this comment.
I brazenly stole this code, because I knew it is working for years and I also knew that I couldn't do better.
But I don't just want to pirate Stephen Harris' code, without proper attrribution - or rewriting it!
Original PR description
As a proof-of-concept PR, this code needs for sure some orchestration!
The used anonymous functions are my preferred way of experimenting, but not meant to stay.
I can see 3 different ways to go on:
class-add-to-calendar.phpclass-rewrite-api.phpthat handles all the rewrites stuff and is only used by the currentclass-event.phpclass-endpoints.php, that could also be the future home for Add subscribeable Ical feeds #603Following path number 2 could be interesting for the future, as it could be reused, if GatherPress would want to create
/ticketsor/qrendpoints on events, for example. Long time ago created a similar abstraction to route AJAX requests, because it was (maybe still is) usually around 10 times faster than the REST API.———
I’ve also tried using
add_rewrite_endpoint()in the first run, but this would have added the new endpoints to allEP_PERMALINKrewrites, which would have meant: all non-hierarchical post types. In that case, I would have had to set custom 404s on all non-event post types, which ended up being very tedious and a lot of additional code. That’s why I went withadd_rewrite_rule()which allows a restriction on one post type.Description of the Change
This PR add several new URL endpoints to events, one per each add-to-calendar link. The new endpoints are:
example.org/event/my-sample-event/icalexample.org/event/my-sample-event/iCal(Alias for/ical)example.org/event/my-sample-event/outlook(Alias for/ical)example.org/event/my-sample-event/googlecalendarexample.org/event/my-sample-event/yahoocalendarWith the original intention to help with #704, this PR also does some more subtle changes to the generated ics file, like changing its default line breaks.
Because this renders into a sites frontend output, this PR additionally introduces a way to overwrite the generated iCal via a theme template. This opens another way of extension and can be used to test this PR.
Can be stated as totally outdated!
Unfortunately, this PR does NOT help with #704,
but instead has grown from 193 to 2607 lines of code and now closes #603 instead.
An extensive description can be found in a new
/docs/developer/custom-url-endpointsarticle, that is also part of this PR.This PR introduces the following of new endpoints:
example.org/event/my-sample-event/icalprovides a download-able .ics file in ical format.
example.org/event/my-sample-event/outlookprovides the same download-able file as an alias.
example.org/event/my-sample-event/google-calendarredirects to create a new event in Google Calendar.
example.org/event/my-sample-event/yahoo-calendarredirects to create a new event in Yahoo Calendar.
example.org/event/feed/icalprovides a subscribe-able event feed in ical format with all events of the site.
example.org/venue/my-sample-venue/feed/icalprovides a subscribe-able event feed in ical format with all events at that venue.
example.org/topic/my-sample-topic/feed/icalprovides a subscribe-able event feed in ical format with all events grouped into that topic.
How to test the Change
echo ‚hello themed iCal world‘;into itChangelog Entry
Credits
Props @carstingaxion
Checklist:
/readme.md/docs/contributor/unit-tests/docs/developer/custom-url-endpoints/docs/developer/theme-customizationsRelated PRs that can be reviewed & merged after this:
To get the new endpoints into the block-editor, one of the following would be required:
OR