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

Prevent $pages->add() from silently breaking when adding invalid entries #1890

Closed
hdodov opened this issue Jul 1, 2019 · 5 comments

Comments

@hdodov
Copy link

@hdodov hdodov commented Jul 1, 2019

Describe the bug
I want to create a pages collection containing ModelWithContent entries. However, if I add the $site, I run into problems, probably because it doesn't have an ID.

I know I could use Kirby\Cms\Collection, but I need the index() method which is only present in Kirby\Cms\Pages.

To Reproduce
To reproduce the behavior:

$models = new Kirby\Cms\Pages();
$models->add(site());

Expected behavior
The collection should contain the Site.

Kirby Version
3.2.0

Additional context
I get that this is kind of an edge case, but Kirby is all about flexibility. Since both Site and Page extend ModelWithContent, it is expected that you can create a group of ModelWithContent that can contain both of these child classes.

My use case is for a plugin - the user can provide a Pages object and anything inside is exported. He might want to include the Site data there or he might not.

Note: It works OK if I use prepend()/append() instead of add(). Judging by the source code, it may be because Site doesn't have an ID. If that's the problem, then my proposal is to give it an ID of <site> or something similar.

@neildaniels

This comment has been minimized.

Copy link
Contributor

@neildaniels neildaniels commented Jul 1, 2019

I wouldn't expect that a Pages object would hold anything that isn't a Page—namely because so many Pages methods don't make sense with a Site in the mix.

If you need $pages->index() why not get the index of pages first, add that to a Collection then add the Site to that collection? Calling $pages->index() with a Site in there seems redundant, because $site->index() would include literally all pages.

@hdodov

This comment has been minimized.

Copy link
Author

@hdodov hdodov commented Jul 2, 2019

@neildaniels hm, I agree, that actually does make more sense. In that case, $pages->add(site()) should throw an error, instead of silently breaking. Something like "Pages must have an ID".

Edit: It also breaks silently when you do:

$pages->add([
  'foo' => 'bar'
]);

I wouldn't expect that a Pages object would hold anything that isn't a Page

Then Pages should not allow anything that isn't a Page.

@hdodov hdodov changed the title $pages->add() doesn't work with $site Prevent $pages->add() from silently breaking when adding invalid entries Jul 2, 2019
@neildaniels

This comment has been minimized.

Copy link
Contributor

@neildaniels neildaniels commented Jul 2, 2019

I actually like the current behavior, because I can do things like $pages->add($field->toPage()) and not have to check if toPage even returns null or not. I suppose null could be an allowed case though.

@hdodov

This comment has been minimized.

Copy link
Author

@hdodov hdodov commented Jul 2, 2019

Well, you need to check for null when using a method of the resulting toPage() object:

if ($myPage = $field->toPage()) {
  echo $myPage->url();
}

Having to check for null in other circumstances would make things more consistent. I get what you're saying, but imagine if add() fails silently when it receives null and you're not expecting it. It could be a pretty hard thing to debug.

Besides, I'm not sure that putting off handling of invalid values to underlying APIs is a good pattern. It could lead to many unexpected behaviors in my opinion.

That might be OK for other cases, but for this one, as you've already said, a Pages object should contain Page objects. Also, if you add(null), the size of the collection hasn't changed and you technically did not add anything, so it doesn't make sense for that to run successfully.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Oct 15, 2019

I think we found a good solution for it. You can still pass null or false and those values are simply ignored, but when you try to pass an invalid object, which is not a Page, an exception is thrown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.