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

improve extensibility and optimize stored data #21

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cekk
Copy link
Member

@cekk cekk commented Nov 9, 2021

Two things:

I moved columns and fields out to be easily customized by a more specific adapter.

In redturtle.volto we do not store the whole object data in blocks when it's an internal link reference because infos in that object can be dynamic (for example the content has been renamed).
In deserializer we store only an object with the UID and in the serializer we expand it with ISerializeToJsonSummary/ISerializeToJson.

We store an object like this:

{"UID": "whatever"}

but in plone.volto you are storing the uid in @id..let's define a standard and i will fix it ;)

Are you interested in using the same pattern also here? Do you see some possible problems?
Is it better to expand items as summary (less fields) or having the full data?

@tiberiuichim @tisto @sneridagh

@mister-roboto
Copy link

@cekk thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@tiberiuichim
Copy link
Contributor

I like the idea, I think it's good.

@sneridagh
Copy link
Member

I like it, but it will force us to rewrite a good amount of blocks (and migrate them!), isn't it? We could write the serializer in a way that it's still compatible with the old way, then deserialise to the new one.

@tiberiuichim
Copy link
Contributor

@sneridagh agree, let's put it on the roadmap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants