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

Issue #139: Use JsonEncoded everywhere possible. #140

Closed

Conversation

Joaoeduardoramos
Copy link

Please read description here: #139

@vever001
Copy link
Contributor

Just tested this, everything looks good IMO.
Behat and PHPUnit still green locally.

This simple change gives the ability to alter any Webtools JSON content with minimal effort on both sides (oe_webtools and consumers/project owners).
So huge +1 on this.

@vever001
Copy link
Contributor

I see the only place where this was not applied yet is in oe_webtools_analytics_page_attachments().
Is it relevant to apply JsonEncoded there as well? Since there is already an event...

@Joaoeduardoramos
Copy link
Author

I see the only place where this was not applied yet is in oe_webtools_analytics_page_attachments(). Is it relevant to apply JsonEncoded there as well? Since there is already an event...

Hi Hervé, I completely missed this comment, a thousand pardons. For me it can stay as it is!

@Joaoeduardoramos Joaoeduardoramos force-pushed the issue_139 branch 3 times, most recently from 0b72786 to 03697e1 Compare May 23, 2022 15:24
@vever001
Copy link
Contributor

vever001 commented May 23, 2022

Hi Hervé, I completely missed this comment, a thousand pardons. For me it can stay as it is!

Hi Joao, no worries, this was an explanation mainly for oe_webtools maintainers. It can stay as is.

BTW, I think we could improve further that JsonEncoded class by implementing ArrayAccess, similar to \Drupal\Core\Template\Attribute
That would make it even better but it doesn't necessarily need to be done in this PR.
Hopefully someone will look at this PR because I think it is a great addition in terms of DX and ease of alteration.

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.

2 participants