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

[RFC] Thoughts on deprecating all methods on the Content object #6985

Closed
rossriley opened this issue Sep 8, 2017 · 11 comments
Closed

[RFC] Thoughts on deprecating all methods on the Content object #6985

rossriley opened this issue Sep 8, 2017 · 11 comments
Labels
RFC Request For Comments stale Stale issues & PRs flagged for closing

Comments

@rossriley
Copy link
Contributor

This may not be desirable or doing it now for deprecation in 4.0 might be deemed to be too hasty so I'm just interested in getting some general feedback before I start working on anything.

As you know I'm working on the compatibility between old and new storage layers and one of the biggest jobs is making sure that a new content object behaves as similarly as possible to an old one.

One of the downsides of this is that these content objects need to know too much and do too much and if we want to improve things in the future I'm suggesting that we standardise our public Twig API on functions and filters rather than having core functionality mixed into the content objects themselves.

The other part is that it's very hard to determine what is and what isn't the public API and ideally when 4.0 hits we want to reduce the number of surprises as people migrate their themes over.

So the main methods to address are:

record.link()
record.previous()
record.next()
record.excerpt()
record.fieldtype()

Also perhaps have a pass through the classes and traits and mark unnecessarily public methods as protected.

The idea would be to provide this functionality via functions or filters where applicable and these can then have the correct dependencies injected into them.. for example something like...

record|link or link(record)
record|link('next') or link(record, 'next')
record|link('previous') or link(record, 'previous')
record|excerpt or excerpt(record)
fieldtype(record, 'fieldname')

Obviously these are just initial suggestions and details can be hammered out in implementation phase.

Thoughts / alternative ideas welcome.

@GwendolenLynch GwendolenLynch added the RFC Request For Comments label Sep 8, 2017
@GwendolenLynch
Copy link
Contributor

Related #6774 and dev meeting discussion from 11 July

@GwendolenLynch
Copy link
Contributor

As I raised in #6774 and have become pretty firm on, I think your suggestions here reflect a good way to go.

I am not fussed if they are filters, functions, or both. But there absolutely needs to be a way forward now so, as you said, there is less work/surprise in the future for people.

@xiaohutai
Copy link
Member

Also related #6798. In combination with Twig.

@stale
Copy link

stale bot commented Nov 20, 2017

This issue has been automatically marked as stale because it has not had recent activity. Maybe this issue has been fixed in a recent release, or perhaps it is not affecting a lot of people?
It will be closed if no further activity occurs, because we like to keep the issue queue concise and actual.
If you think this issue is still relevant, please let us know. Especially if you’d like to help fix the issue, either by helping us pinpointing the cause of a bug, or in implementing a fix or new feature.

@stale stale bot added the stale Stale issues & PRs flagged for closing label Nov 20, 2017
@SvanteRichter
Copy link
Contributor

@bobdenotter @GawainLynch @rossriley @CarsonF did we land on the |explain idea and deprecate/remove here or are there any objections?

@stale stale bot removed the stale Stale issues & PRs flagged for closing label Nov 20, 2017
@rossriley
Copy link
Contributor Author

I am going to be working on this for 3.5, have a branch in progress now so happy to close this. Can't remember what was discussed about the explain filter what did we decide?

@SvanteRichter
Copy link
Contributor

SvanteRichter commented Nov 20, 2017

IIRC the compromise plan that everyone was okay with was to have a |explain filter in twig which would show what could be accessed from the content object (like relations, links and so on).

I'm pretty sure @CarsonF and me talked about implementation details, but those are lost to the slack-iverse.

@bobdenotter
Copy link
Member

I was initially pretty much against deprecating them. Unless we have an alternative that is.

So, let's deprecate them, if we can provide users with a way to still "dump" the entire contents of a record, to see what's in it.. Thinking out loud, would {{ record|inspect }} be a better name than explain?

Also, it'd be great if we can extend this to work for multiple records: {{ records|inspect }}.

@SvanteRichter
Copy link
Contributor

@bobdenotter Yeah, the idea was for |explain to be sorta like a dump but with better info on what can be done with a record or other object (or a collection of objects).

We had a discussion in slack about naming and I think there was some argument against |inspect, but I can't remember what.

@stale
Copy link

stale bot commented Jan 20, 2018

This issue has been automatically marked as stale because it has not had recent activity. Maybe this issue has been fixed in a recent release, or perhaps it is not affecting a lot of people?
It will be closed if no further activity occurs, because we like to keep the issue queue concise and actual.
If you think this issue is still relevant, please let us know. Especially if you’d like to help resolve the issue, either by helping us pinpointing the cause of a bug, or in implementing a fix or new feature.

@stale stale bot added the stale Stale issues & PRs flagged for closing label Jan 20, 2018
@rossriley
Copy link
Contributor Author

Ok, well the initial part of this is completed, in 3.x .previous .next .editlink etc will still all work on entities but they forward to twig filters and functions and thus this can become the official way as of 4.0.

Not sure on what we want the spec for the explain filter idea but we can perhaps spec that up and PR separately. Closing this as a result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request For Comments stale Stale issues & PRs flagged for closing
Projects
None yet
Development

No branches or pull requests

5 participants