ideas icon indicating copy to clipboard operation
ideas copied to clipboard

Allow access to content fields through model properties

Open qwerdee opened this issue 5 years ago • 11 comments

The most convenient way of accessing content fields is through magic methods like $page->body()->kt(). There are some caveats thought, some of them are also mentioned in the docs.

For one there are naming collisions with public model methods. You can't access fields that way if there is a method on the model with the same name. The docs point to using the corresponding content object for these cases instead like $page->content()->get('template').

Second, the way these magic methods work lead to inconsistent casing. It is recommended in the docs to use snake_case for field names which is in line with database attribute naming conventions. However, all methods are in camelCase, so seeing something like $page->related_articles()->toPages() looks not right. You have to know about this detail to not get confused here.

As a possible improvement to these point we could give access to model fields through model attributes via magic getter. That way there is less chance of naming collisions since public properties on models are way rarer than public methods. Further it would give a nice distinction between methods and fields. With this distinction I believe having different casing looks less weird and is easier to explain, where to use what. It would look like following:

<?php
// Proposed Solution:
$page->my_field->toHtml();

// in addition to:
$page->my_field()->toHtml(); // <-- weird mixed casing, not immediately clear why
$page->content()->get('my_field')->toHtml(); // <-- very precise and clear but also verbose

It seems to be working fine, I'm gonna prepare a PR for this. May be there is a better way to solve those issues? Can you think of implications of this change?

qwerdee avatar May 18 '20 10:05 qwerdee

I don’t see how the solution would solve the outlined issue of mixed casing - that would be the same.

And with properties you would still have have naming collisions with the existing properties of these classes, just different collisions.

distantnative avatar May 18 '20 10:05 distantnative

You are right, but it would at least improve it since there is an obvious distinction between methods and properties. Therefore it would make it easier to reason about, which case to use.

Another solution would be to allow camelCase for the short cut methods so that $page->myField() and $page->content()->get('my_field') would be the same.

qwerdee avatar May 18 '20 10:05 qwerdee

In regards to the naming collision, as mentioned, this also would not solve it completely, just reduce the chance. I just checked the Page Model and it has only six public properties. Definitely less than public methods.

qwerdee avatar May 18 '20 11:05 qwerdee

It's a clever hack, but I think mixing the syntax in a method chain looks weird and people with little PHP knowledge won't be able to tell what the difference is and how and why this works.

So what we could do is to have this in as an advanced feature for alternative content field access, but I don't think this can and should be documented throughout the docs for the abovementioned reasons.

Note that you can already use magic methods on the Content object:

$page->content()->my_field()

Of course this still has the underscore issue, but after all that's a different issue that also won't be solved by the magic getter suggestion (IMO properties and methods with underscores look equally weird).

lukasbestle avatar May 18 '20 18:05 lukasbestle

On a side note: Where in the docs do we recommend using underscores in field names? I never do…

texnixe avatar May 19 '20 08:05 texnixe

@lukasbestle Yes totally, it could be added without interfering the official usage via methods. For me personally it looks weirder to mix casing between methods in a method chain. With properties the field names are immediately distinguishable even in a chain. Of course you still have to know why there is a different casing but you get a better hint of where this applies.

Consider this:

<?php

$page->related_pages()->toPages()->first()->is_feature()->isTrue();

$page->related_pages->toPages()->first()->is_feature->isTrue();

qwerdee avatar May 19 '20 09:05 qwerdee

@texnixe It is a rather small note here in the guide: https://getkirby.com/docs/guide/blueprints/introduction#tips-tricks__naming-fields

qwerdee avatar May 19 '20 09:05 qwerdee

I updated the PR so it works with structured field content in the same way

qwerdee avatar May 19 '20 09:05 qwerdee

@qwerdee But that is about dashes, not using camelCase or being forced to using underscores.

texnixe avatar May 20 '20 12:05 texnixe

@texnixe ok, maybe I interpreted this the wrong way.

Make sure to only use alpha-numerical characters and underscores in field names

I thought this would imply to use snake_case for field names. Can I ask, what is the recommended way for multi word field names if not snake_case?

I know, that camelCase technically works however the word separation vanishes in the content text files, which I think is not ideal (blueprint: someField=> content: Somefield). But of course then using camelCase in the methods works, since they are implemented case insensitive.

qwerdee avatar May 20 '20 13:05 qwerdee

camelCase in blueprints is indeed the variant I'd use as well. It can be used everywhere in your code and will always work (there are a few inconsistencies at the moment, but we will fix those in 3.5.0). You are right that the content files will always use Beginninguppercase, but to be honest I think that this is not too problematic.

lukasbestle avatar May 20 '20 16:05 lukasbestle