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

MagicContentTrait DX improvements #151

Closed
JarJak opened this issue Nov 22, 2018 · 25 comments

Comments

@JarJak
Copy link
Member

commented Nov 22, 2018

Splitted from #148

  • I really don't like all those "magic" methods in ContentMagicTrait. Can we at least rename them to be less magical? :)
  • if we could make $this->fields a name-indexed array, using this method be much faster (simple isset instead of foreaching).
  • rename to get to getField, has to hasField
@JarJak

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

Also in MediaFactory:

  • can you replace it with an \Exception and add @todo? At least for getting trace where it happened :) done
@bobdenotter

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

I really don't like all those "magic" methods in ContentMagicTrait. Can we at least rename them to be less magical? :)

In Bolt 3 we also had these, only they weren't named as such. And IMHO that gave a bigger problem. When you'd get title, you'd either get the magic title-like result, or the actual field named title. It was a source of confusion.

I've named them like this to make it explicit.. You can now fetch what you need, and you'll know what to expect as the result.
What would you change it to, instead?

if we could make $this->fields a name-indexed array, using this method be much faster (simple isset instead of foreaching).

Yes, certainly.. What we have now is basically the generated piece of code from make:entity.
I was thinking about turning it into a Collection, too.

rename to get to getField, has to hasField

Will do.

can you replace it with an \Exception and add @todo? At least for getting trace where it happened :)

Allright, allright.. ;-)

@JarJak

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2018

What would you change it to, instead?

I would like to get rid of them, completely. I don't feel they are really needed (at least inside an entity, they can be moved to some service).

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

I don't feel they are really needed

The are needed on the overview pages to show titles, excerpts, thumbnails. They are needed in the sidebar menu to link the 'last 5 items'.. In the "frontend" they're used for listing pages as well as 'generic content pages'.

I'm open to renaming them, but they are most certainly required. :-)

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

if we could make $this->fields a name-indexed array, using this method be much faster (simple isset instead of foreaching).

Over the weekend i worked on getLocalisedFields() and related methods. These return named fields, as a Collection.

@JarJak

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

I know they are used here and there. But an Entity is not a good place for logic like this. That's why I'd vote for moving them away. It breaks SRP, Law of Demeter and probably a few other OOP rules.

OR

Add tests to cover every use case of those service-alike entities ;)

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

That's why I'd vote for moving them away.

Not opposed to that. How would you do this? To me, it's very important that it doesn't detract from the theme/site implementors ease of use that can now simply use {{ record.excerpt }} and know they'll get an excerpt.

Add tests to cover every use case of those service-alike entities

Excellent point. We absolutely need to get started on adding unit tests too. :-)

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

See #168

@JarJak

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

How would you do this?

Entity Decorator with logic needed for things like getLink or Excerpt.

OR

use twig filters instead:
{{ record | excerpt(200) }}
{{ record | link(absolute=true) }}

I would vote on the second as it will be still easy for frontend devs. @bobdenotter how about you?

@JarJak

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

And then we could get rid off service container inside an entity

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

I (personally) don't really like that. It is too big a break from what we've used before, and it looks less clean i think..

{{ record.link() }} -> is the link of a record
{{ record|link() }} -> is a record processed into a link.

I'll ask my colleagues what they think as well!

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

Additionally, for the sake of conversation: Let's say we make all the current "magic" methods into filters.. How would they be usable in an API / JSON context?

@anketwokings

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

As a frontenddeveloper (and colleague of Bob, who asked me to take a look at this as well), I am in favour of less magic.

I don't know any technical implications, but I'd prefer to get a field when I ask for a record.link, and a generated thing when I ask for record | link().

I sometimes stumble upon code where Bolt magically tries to guess what I want, when I think i clearly ask for a field, so I like @JarJak s solution. :)

@sbonardt

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

I'm in favour of no new custom filters. Also I feel using the '.' with () gives it a more of function approach/feel to it.You have a record and .link() gives you the link to it.

A record where you |filter the link doesn't feel right to me somehow.
I filter stuff off of values, like record.title|trim('/')

So I'm with team .link()

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

@anketwokings The way it was "magic" in Bolt 3 is mostly solved because the current way in the prototype makes it a lot more consistent.

What you describe as your problem is a result of a faulty implementation. With the correct implementation of it, I think it's more elegant than a bunch of filters.

@jadwigo

This comment has been minimized.

Copy link

commented Jan 21, 2019

I like it - even if it changes stuff.. Bolt 4.x will not be 100% compatible with Bolt 3.x anyway.

So {{ record.link }} gives me the link field in the record and {{ record|link() }} generates a link for the record based on the record content and maybe some more information in the configuration.

Contrary to what @sbonardt says I like the part where {{ record.link }} gives me a value and {{ record|link() }} does something with a record to give me something (which is what a filter does).
Stuff like {{ record|title()|trim('/') }} still works and is problably more what twig expects than the dot notation.

Whith filters you can also make variations like {{ record|link('fqdn') }} generate https://www.example.com/record/slug and {{ record|link('relative') }} generate /record/slug without much semantic overhead for front-end devs.

This will make life for front-end devs who work in the twig templates much easier compared to bolt 3 where the magic is getting in the way sometimes.

For people who create the back-end templates the JSON probably needs changes, but the whole back-end is new anyway so it does not really matter if we change it to one option or the other.

I would choose the version that makes life better for the front-end devs ({{ record|link() }}) at this point, because they are many and the rest of the devs are few.

@anketwokings

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

@bobdenotter Had to dig, but I encountered it only one time that I can remember; Old issue alert -> bolt/bolt#6798

@xiaohutai

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

I like the |link() solution, it's (technically) clear what it does. Only thing is that it initially may not be clear to someone who knows a bit less about Bolt ("What's the difference? Why?"). But the distinction will be:

  • a contenttypes.yml defined field, use .fieldname
  • every internal/magic function is |link or |link()

My reasons:

  • I want to be able to use field names like link
  • I don't want some random magictitle derivate values in my instances
  • I think a Content object should be really stupid (less magic, more predictable)

For API/JSON context, I think there needs to be a function that transforms a Content instance to proper JSON. E.g. for JSON API, you need something like this:

{
    "data": [{
        "links": {
            "self: {{ record|link() }}"
        },
        "attributes": {
            "title": "{{ record.title }}",
            "foo": "{{ record.foo }}",
            ...
        }
    }]
}

Do note that in this case, you need to think of a different way to generate titles/links in PHP.

@JarJak

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

@bobdenotter API output will not be a problem. It can include and exclude anything or be something totally custom.

Doctrine's Entity is tightly coupled with the Persistence Layer. So it should stay as simple as possible. Then we can make another (domain) layer where we can put any logic we want. Then we go to Presentation layer - what API actually outputs. I would recommend avoid mixing those layers (call them MVC, ADR, CQS, whatever).

@JarJak

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

Another thing I have noticed: ContentmagicTrait uses Twig_Markup so we are mixing M with V layers there.

@JarJak

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

bolt/bolt#6985 similar discussion from a year ago

@JarJak

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

Ok this is what I would propose:

  1. Move "magic" methods: link, editLink, excerpt, iimage, previous, next - to Twig filters, so they become:
  • {{ content | link }} or {{ content | link() }} or {{ content | link(absolute=false) }} gives relative link
  • {{ content | link(absolute=true) }} or {{ content | link(true) }} gives absolute link
  • {{ content | edit_link }} will work the same (relative/absolute) but it COULD be restricted to work only for logged-in users
  • {{ content | magic_image }} useful in Backend (MAYBE we could add here {{ content | magic_image(field=another_image_field) }} ?)
  • {{ content | magic_title }} useful in Backend
  • {{ content | excerpt }} or {{ content | excerpt(length=300) }} useful in Backend
  • {{ content | next_record }} and {{ content | previous_record }}
  1. Let's think about API output for get_content endpoint, how could it look like with magics included? I would recommend including only link and editLink as the others are more needed in backend than frontend. So it can become something like:
{
    "id": 3,
    "slug": "random-slug"
    "contentType": "blogpost"
    "fields": {
        "title": "Random title"
        "teaser": "...",
        "image": {
            "url": "..."
            "alt": "..."
        }
    }
    "extras": {
        "link": "magicLink here"
        "editLink": "magicEditLink here"
    }    
}

It's just an example. Due to API Platform elastic mechanisms it can look just like frontend devs would love it :)

  1. Same thing applies to serialized Content object. I would recommend to make it look same (or almost same) to what API returns.

  2. Last but not least, all those magic domain logic will be separated from Model (Doctrine entities) and View (Twig filters will be just aliases to services, API views will just serialize domain models here). Also since we will get specialized services for those tasks then will be unit testable.

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Ok, if the consensus is that |filter is a good solution, and we can keep them in the "extras" in JSON / API, let's roll with it!

One small remark: magic_image, magic_title and excerpt are used in the frontend as well:

  • In the "out of box" experience, where it "just works" when you install it, and have some random content
  • In listings like search results, where we display different types of content mixed together, regardless of which fields they have
  • In "RAD", because it's easy to just use the 'record.twig' at first, and replace it with a custom template later.

So, we should just make them available in the frontend as well, and i'm all good. 👍

@JarJak

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

@bobdenotter ok but do you include headless frontend?

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Headless frontend is considered JSON / API, afaic. So, if we can include them in the "extras" for those usecases, that's great. :-)

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