Replace command with WP Cron + REST endpoints #7
Conversation
mvdhoek1
commented
Jul 12, 2024
mvdhoek1
commented
mvdhoek1
requested review from
sanderdekroon,
dtakken,
robertbossaert,
ictbeheer,
SimonvanWijhe and
projmunoz
| * | ||
| * @return mixed | ||
| */ | ||
| public function handleRequestParam(WP_REST_Request $request, string $param, string $type, $default = null) |
There was a problem hiding this comment.
Als je args definieert in register_rest_route gebeurt dit allemaal automatisch. (validatie, default enz)
https://developer.wordpress.org/rest-api/extending-the-rest-api/schema/
There was a problem hiding this comment.
Ah goeie, ga ik niet nu meenemen. Wellicht in de volgende versie.
| */ | ||
| public function getLegeBySlug(WP_REST_Request $request) | ||
| { | ||
| $slug = $request->get_param('slug'); |
There was a problem hiding this comment.
Waarom hier geen gebruik van $this->handleRequestParam()?
| { | ||
| $perPage = $query->get('posts_per_page'); | ||
| $page = $query->get('paged'); | ||
| $page = 0 == $page || -1 == $perPage ? 1 : $page; // If $perPage = -1, $page should be 1. |
There was a problem hiding this comment.
Ik hou ook van compacte code, maar ik vind dit wel moeilijk leesbaar. Haakjes maken het (voor mij althans) iets beter leesbaar: $page = (0 == $page || -1 == $perPage) ? 1 : $page;.
|
|
||
| protected function timeToExecute(): int | ||
| { | ||
| $currentDateTime = new DateTime('now', new DateTimeZone(wp_timezone_string())); |
There was a problem hiding this comment.
wp_timezone_string() kan ook een offset teruggeven (bijv. +08:45). Zo'n waarde is niet in een format die door DateTimeZone geaccepteerd wordt ("One of the supported timezone names, an offset value (+0200), or a timezone abbreviation (BST)"). Wellicht hier een try...catch omheen zetten.
There was a problem hiding this comment.
Goed dat je het zegt, dit is een betere oplossing:
$currentDateTime = new DateTime('now', wp_timezone());
| { | ||
| $slug = $request->get_param('slug'); | ||
|
|
||
| if (empty($slug) || ! is_string($slug)) { |
There was a problem hiding this comment.
Dit gaat ook vanzelf als je slug als arg registreert.
mvdhoek1
force-pushed
the
feat/wp-cron-event-updating-leges
branch
from
3220af4 to
130f161
Compare
CHANGELOG.md
Outdated
| @@ -1,94 +1,106 @@ | |||
| # CHANGELOG | |||
|
|
|||
| ## Version 1.2.5 | |||
| ## [2.0.0] | |||
There was a problem hiding this comment.
Top om die Version hier weg te laten maar waarom deze brackets?
| @@ -4,7 +4,7 @@ | |||
| * Plugin Name: PDC Leges | |||
| * Plugin URI: https://www.openwebconcept.nl | |||
| * Description: PDC Leges | |||
| * Version: 1.2.5 | |||
| * Version: 2.0.0 | |||
There was a problem hiding this comment.
Just checking: is dit een major, backwards incompatible version? Is er een upgrade guide nodig?
There was a problem hiding this comment.
Nee, het huidige commando (<2.0) is straks weg maar zal niets breken, in de README.md staat het wel beschreven:
Since version 2.0.0, the commands have been replaced by WP Cron Events. This change requires less configuration on your server by eliminating the need to add server cron jobs. Just activate the plugin and you're all set.
Remember to remove any previously configured cron jobs from your web server, as they have been deprecated since version 2.0.0.
mvdhoek1
force-pushed
the
feat/wp-cron-event-updating-leges
branch
from
130f161 to
4695448
Compare
mvdhoek1
force-pushed
the
feat/wp-cron-event-updating-leges
branch
from
4695448 to
984259c
Compare
| 'type' => 'integer', | ||
| 'default' => 10, | ||
| ], | ||
| 'page' => [ |
There was a problem hiding this comment.
Ik zou hier nog 'minimum' => 1 toevoegen
| 'permission_callback' => '__return_true', | ||
| 'args' => [ | ||
| 'limit' => [ | ||
| 'description' => 'Number of posts per page.', |
| 'description' => 'Slug of the post.', | ||
| 'type' => 'string', | ||
| 'default' => '', | ||
| ] |
There was a problem hiding this comment.
Als je slug 'required' => true maakt hoef je in de controller niet meer te checken of ie gevuld is
There was a problem hiding this comment.
Zal niets doen, zonder slug param komt ie in het algemene endpoint voor het ophalen van alle leges.
mvdhoek1
force-pushed
the
feat/wp-cron-event-updating-leges
branch
from
3b119b9 to
889cae7
Compare
mvdhoek1
force-pushed
the
feat/wp-cron-event-updating-leges
branch
from
889cae7 to
daf843e
Compare
