LPS-90405 Remove parser and loader from Soy #1236
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
|
ci:test |
liferay-continuous-integration
added
ci:test - pending
ci:test - failure
and removed
ci:test - pending
labels
❌ ci:test - 56 out of 184 jobs passed in 2 hours 39 minutes 30 seconds 274 msClick here for more details.Base Branch:Branch Name: master Copied in Private Modules Branch:Branch Name: master-private 128 Failed Jobs:
56 Successful Jobs:
For more details click here.Failures unique to this pull:
|
|
ci:test |
tinatian
reviewed
Feb 15, 2019
| } | ||
|
|
||
| return templateResource; | ||
| return new URLTemplateResource(templateId, url); |
There was a problem hiding this comment.
Hi @dantewang,
Before, we should go into the first branch which is using TemplateResourceLoaderUtil to generate the URLTemplateResource.
After, you create URLTemplateResource directly.
The result should be same, but the difference is TemplateResourceLoaderUtil has a cache level, I think this is going to affect performance.
I searched for the usages in our code base, and found a lot of usages like this, that is to create a URLTemplateResource directly instead of using TemplateResourceLoaderUtil. This should be wrong, but we can not stop people doing so since URLTemplateResource is in kernel package.
An option to avoid URLTemplateResource created directly is to replace all the usages to use TemplateResourceLoaderUtil, and copy it to internal package and deprecate the kernel one.
The problem with that option is currently TemplateResourceLoaderUtil does not export easy API for people to use. Like in this example, if we use TemplateResourceLoaderUtil, we have to manipulate the template id and parse it to create URL, just like what we did in LPS-88867.
Could you please check all the usages of creating template resource directly, and see if we can create API for those usages, and then remove all the direct creating and deprecate the kernel class.
We should not do the removing like you did in this pull, so I will close it.
Thanks.
Tina.
|
@tinatian
Yes the other usages have this problem.
But Soy seems different. From my digging and debugging this code in Soy is
executed when a bundle with Soy capability is deployed. Once the template
resource instances are created for such a bundle, they are held by Soy
until the bundle is undeployed. Even if the loader exists for Soy, there
will be no invocation to the loader at all once the bundle deployment is
done.
So Soy isn't leveraging the cache layer in the loader at all both before &
after, it does caching itself.
Tina Tian <notifications@github.com> 于 2019年2月16日周六 02:17写道：
… Closed #1236 <#1236>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1236 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABIVJ1LTlJH_jql4RCpJSHMSZgdneyjeks5vNvm7gaJpZM4a190o>
.
|
|
@dantewang According to the code, it seems it can go to TemplateResourceLoader, I think the reason you did not see it hit the branch is because the order of services. That is the bundle with soy template is activated before the soy template bundle itself. Why this happen? It is because it is using TemplateResourceLoaderUtil instead of depending on the specific reference. It should have a dependency on the soy template resource loader, but it is not, so here, they have to do the checking This is wrong, for modules, it should not use ***Util, it should depend on specific service directly, the ***Util is just a bridge for the code in portal core. Tina. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@tinatian As confirmed with Soy team, these can be removed since Soy goes with bundle scanning upon activation approach.
cc @jbalsas @izaera @brunobasto