Add support for macros #329

2020-06-11T16:04:13Z

matthijskooijman commented

2020-06-11 16:04:13 +00:00

(Migrated from github.com)

This is still a WIP, just wanted to share my progress so far.

matthijskooijman commented

2020-06-23 16:54:55 +00:00

(Migrated from github.com)

I've now added a page list macro, which can be used to generate e.g. a homepage with recent articles, or a tag-based index page. Used e.g. like:

<macro name="pagelist" tag="en/tag_label" pagetypes="peer_reviewed_article" languages="en" include-private="yes" sort="-date" limit="10" />

This probably still needs some more helper methods to make accessing macro options easier and more robust (though there is already some error checking in place).

I've now added a page list macro, which can be used to generate e.g. a homepage with recent articles, or a tag-based index page. Used e.g. like: <macro name="pagelist" tag="en/tag_label" pagetypes="peer_reviewed_article" languages="en" include-private="yes" sort="-date" limit="10" /> This probably still needs some more helper methods to make accessing macro options easier and more robust (though there is already some error checking in place).

matthijskooijman commented

2020-10-12 15:42:58 +00:00

(Migrated from github.com)

I think this is ready for merging now. I've used the pagelist macro to automate the homepage lists (both the audio/video as well as the list of normal articles) at http://test.destadsbron.nl/nl/home

Review of the code, but also the test site is welcome.

I think this is ready for merging now. I've used the pagelist macro to automate the homepage lists (both the audio/video as well as the list of normal articles) at http://test.destadsbron.nl/nl/home Review of the code, but also the test site is welcome.

matthijskooijman commented

2020-10-21 12:53:55 +00:00

(Migrated from github.com)

I added some more options to the pagelist macro, it should now be suitable to be used on e.g. De Stadsbron to generate the homepage, tag pages and timeline/archive pages. I'll probably deploy this code already, while waiting for review and merging.

RoukePouw (Migrated from github.com) reviewed

2020-12-23 09:54:01 +00:00

system/core/macros.php

					
				@ -0,0 +80,4 @@

					 * @param string $name The name of the attribute to get

					 * @return DateTime | null

					 */

					protected function getDateTimeAttribute($name, $format, $errorFormat, $default = null) {

I'm a bit surprised to see this function here. I wouldn't expect it to be part of the base class of a macro, seems a bit specific. But perhaps I'm missing something?

RoukePouw (Migrated from github.com) reviewed

2020-12-23 10:15:42 +00:00

system/macros/pagelist.php

					
				@ -0,0 +136,4 @@

							$validTypes = hypha_getDataTypes();

							foreach ($pageTypes as $type) {

								if (!array_key_exists($type, $validTypes))

									throw new UnexpectedValueException("Invalid page type: $type. Valid options are: " . implode(', ', array_keys($validTypes)));

Do we want to abort or fail with a message? If in the future one page type is removed/changed then the entire macro becomes broken. I'm thinking about when a user in a few years looks back at a page made a few years before that,

matthijskooijman (Migrated from github.com) reviewed

2020-12-23 10:18:40 +00:00

system/macros/pagelist.php

					
				@ -0,0 +136,4 @@

							$validTypes = hypha_getDataTypes();

							foreach ($pageTypes as $type) {

								if (!array_key_exists($type, $validTypes))

									throw new UnexpectedValueException("Invalid page type: $type. Valid options are: " . implode(', ', array_keys($validTypes)));

This exception is caught further up, so it's just the macro that fails, not the entire page. That seems to me the safest and clearest way to fail, you could ignore the missing value and render something with a warning on top, but then it's really easy to miss the warning, so I think I'd prefer to just have the macro fail completely.

laurensmartina (Migrated from github.com) approved these changes

2020-12-23 10:23:43 +00:00

laurensmartina (Migrated from github.com) left a comment

Looks great!

system/macros/pagelist.php

					
				@ -0,0 +125,4 @@

						// Include private pages only when logged in and enabled

						// TODO: Better checking of option values (and maybe

						// allow Yes/true/TrUe/etc.)

I think there is a typo here. :-)

matthijskooijman (Migrated from github.com) reviewed

2020-12-23 11:47:38 +00:00

system/core/macros.php

					
				@ -0,0 +80,4 @@

					 * @param string $name The name of the attribute to get

					 * @return DateTime | null

					 */

					protected function getDateTimeAttribute($name, $format, $errorFormat, $default = null) {

Well, I could imagine other macros that also need date/time parameters, so this is just a helper function to access such attributes/arguments. I also anticipate other helper functions for parsing other types of arguments, but I haven't had the need yet I guess.

Maybe it would make sense to move the actual parsing code to a more general place, and then here just call that?

Well, I could imagine other macros that also need date/time parameters, so this is just a helper function to access such attributes/arguments. I also anticipate other helper functions for parsing other types of arguments, but I haven't had the need yet I guess. Maybe it would make sense to move the actual parsing code to a more general place, and then here just call that?

matthijskooijman (Migrated from github.com) reviewed

2020-12-24 11:55:53 +00:00

system/core/macros.php

					
				@ -0,0 +80,4 @@

					 * @param string $name The name of the attribute to get

					 * @return DateTime | null

					 */

					protected function getDateTimeAttribute($name, $format, $errorFormat, $default = null) {

I'm going to leave this for now and merge this. We can always move this code later if that turns out better (e.g. when we add a few more macros).

RoukePouw (Migrated from github.com) reviewed

2020-12-24 11:57:09 +00:00

system/core/macros.php

					
				@ -0,0 +80,4 @@

					 * @param string $name The name of the attribute to get

					 * @return DateTime | null

					 */

					protected function getDateTimeAttribute($name, $format, $errorFormat, $default = null) {

Cool :)

matthijskooijman (Migrated from github.com) reviewed

2020-12-24 12:00:04 +00:00

system/macros/pagelist.php

					
				@ -0,0 +125,4 @@

						// Include private pages only when logged in and enabled

						// TODO: Better checking of option values (and maybe

						// allow Yes/true/TrUe/etc.)

Reading it again, I think that this is not a typo, but I meant to say that checking should be case insensitive :-)

matthijskooijman commented

2020-12-24 14:06:08 +00:00

(Migrated from github.com)

Rebased on top of master, and I added two more fixes:

An unrelated fix for empty index pages I came across
Letting the pagelist filter by the current language always, since this might produce fatal errors otherwise. This is temporary, until #310 is fixed.

With that, I'm merging this.

Rebased on top of master, and I added two more fixes: - An unrelated fix for empty index pages I came across - Letting the pagelist filter by the current language always, since this might produce fatal errors otherwise. This is temporary, until #310 is fixed. With that, I'm merging this.

Sign in to join this conversation.

No reviewers

laurensmartina

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

harmen/hypha!329

Rows
Columns