[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13795705#comment-13795705 ] Gerhard Petracek commented on MYFACES-3797: --- hint for later: we could test all final weld based ee7 servers, if they still have issues with ear-files (see CDI-129). if they still have an issue and we would like to support them, we could refactor MYFACES-3797 to a mixed approach (based on MYFACES-3786). cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Fix For: 2.2.0 Attachments: MYFACES-3797_wrapper.patch with context-param param-nameorg.apache.myfaces.CDI_MANAGED_CONVERTERS_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.CDI_MANAGED_VALIDATORS_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13792814#comment-13792814 ] Leonardo Uribe commented on MYFACES-3797: - I'm starting to think that a solution without use a wrapper is possible, the problem there is conceptual: who handles converter/validator instantiation? CDI or JSF? if is JSF the wrapper should not be needed if things are correctly done (the hack over the state is not required, because only if a converter is marked as serializable or stateholder/partialstateholder, it will be saved/restored), but if is CDI, the wrapper is required to do the gap over state handling. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797_2.patch, MYFACES-3797.patch, MYFACES-3797_with_external_bean_aware_wrapper.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13792901#comment-13792901 ] Gerhard Petracek commented on MYFACES-3797: --- what you are thinking about is more or less what we had to do in codi, however, there is a clear overhead (during every request) and you lose a lot of cdi features. since it's an optional feature, i'll commit the last patch within the next days (if needed we can change it at any time). cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797_2.patch, MYFACES-3797.patch, MYFACES-3797_with_external_bean_aware_wrapper.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13792914#comment-13792914 ] Leonardo Uribe commented on MYFACES-3797: - Could you please wait to commit this feature after the beta? I'm trying to get a beta of 2.2.x out and I would like to have time to study the solution. I'm very close to solve MYFACES-3786 and after that I want to propose a beta release. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797_2.patch, MYFACES-3797.patch, MYFACES-3797_with_external_bean_aware_wrapper.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13792973#comment-13792973 ] Leonardo Uribe commented on MYFACES-3797: - I have checked the last patch and it looks good. It is ok to include it in the beta. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797_2.patch, MYFACES-3797.patch, MYFACES-3797_with_external_bean_aware_wrapper_2.patch, MYFACES-3797_with_external_bean_aware_wrapper.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13789141#comment-13789141 ] Dora Rajappan commented on MYFACES-3797: @Gerard code not affect the state handling unlike the part of inputFile which required wrapper, but makes validator/convertor specific code in delta state helper.(rest of artifacts at least ie ELResolver, PartialViewContext not go via Delta state helper). To avoid specific code in delta state helper, its a good idea to use a wrapper in the least or an adapter at the best for supporting custom implementations , which was not a requirement for part in the case of input file. Its a good idea to enable disable injection for Validator/Convertor. But for rest of artifacts the spec says if they are just mentioned in the config file they are injectable. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797_2.patch, MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13789156#comment-13789156 ] Gerhard Petracek commented on MYFACES-3797: --- since there needs to be a change in the spec. in any case, it doesn't really matter which one it is (a new rule that normal converters and validators are in a wrapper in any case vs. an addition to the api or postponing such instance-creations to PostRestoreState(Event) so that it's independent from a specific state-saving impl.). for the spec. i still prefer an improved api to avoid the need for such mandatory wrappers. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797_2.patch, MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[
https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13787999#comment-13787999
]
Leonardo Uribe commented on MYFACES-3797:
-
I have checked the patch and I can see the point. I think the proposed solution
does not deal with the complexity involved in the state handling. Override
DeltaStateHelper or the state handling code is not the right way to deal with
state handling issues. In this case, I think delegation pattern is the way to
go. What happen if a converter/validator is marked as Serializable? what happen
if a converter/validator is StateHolder? what happen if it is
PartialStateHolder?. What happen if two different views uses the same converter
on the same request? both views will use the same?
I think it is also valid to do something like this:
@SessionScope
public class MyConverterClass implements Converter
{
// some code goes here ...
}
A JSF converter or validator that is marked as a bean with a scope. It should
be valid syntax, and I feel that could be useful.
In conclusion, there are different use cases and first we need to check one by
one how to deal with them. The aim is clear: provide a clean integration
between CDI and JSF.
I would like something like this:
if (cdiEnabled and this object has been injected using a cdi annotation)
{
// ... use a cdi adapter for this stuff or something like that ...)
}
else
{
// ... do it as usual ...
}
I need some time to think about this. First we need to solve MYFACES-3786.
cdi support for converters and validators
-
Key: MYFACES-3797
URL: https://issues.apache.org/jira/browse/MYFACES-3797
Project: MyFaces Core
Issue Type: New Feature
Components: JSR-344
Reporter: Gerhard Petracek
Assignee: Gerhard Petracek
Attachments: MYFACES-3797.patch
with
context-param
param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name
param-valuetrue/param-value
/context-param
and
context-param
param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name
param-valuetrue/param-value
/context-param
it should be possible to enable cdi support for converters/validators.
we need the config, because it was postponed for the spec.
--
This message was sent by Atlassian JIRA
(v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788008#comment-13788008 ] Gerhard Petracek commented on MYFACES-3797: --- #1 the patch doesn't override DeltaStateHelper #2 converter/validator is marked as Serializable: that isn't an issue - in case of normal-scoped beans only the cdi-proxy (which is serializable) will be stored - the cdi-container will handle the rest. in case of serializable dependent scoped beans, we need a 2nd DependentBeanStorage which is session-scoped (not part of the first-draft). #3 converter/validator is StateHolder/PartialStateHolder see UIComponentBase - that's tested already #4 @SessionScope beans @SessionScope is a normal-scope in cdi - we don't have to handle it. the cdi-container will do it. we only see a cdi-proxy for it (which is serializable). cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788029#comment-13788029 ] Radu Creanga commented on MYFACES-3797: --- You might want to take a look at the Omnifaces implementation for this https://code.google.com/p/omnifaces/issues/detail?id=183, maybe there is something useful. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788042#comment-13788042 ] Gerhard Petracek commented on MYFACES-3797: --- @radu: we also have an impl in codi, however, every external impl. has some restrictions. there are workarounds for most of them (but not all). cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788044#comment-13788044 ] Radu Creanga commented on MYFACES-3797: --- @Gerhard Was not aware CODI implemented this. Anyway, was just a thought. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788045#comment-13788045 ] Gerhard Petracek commented on MYFACES-3797: --- np - thx radu for the hint, but within a jsf-impl. it's way easier to cover all cases. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788047#comment-13788047 ] Leonardo Uribe commented on MYFACES-3797: - #1 The patch adds some specific code inside, changing the default behavior of the state handling algorithm. But what happen if the user implements directly a class from UIComponent (for example Trinidad and other jsf libraries)? all that code could be just ignored. Include that code also has performance considerations, because most of the time it makes unnecessary checks. The nice part about delegation pattern in this part is you can encapsulate the code that do the injection or whatever in just one place and the adapter will deal with the state issues (if is serializable, if is stateholder, whatever). The idea is attach a proxy on the component tree that deal with CDI logic in those cases where the converter or validator uses CDI injection. #2 It becomes an issue when you think about pss algorithm. Where a serializable converter should be stored? it the view scope? why there is an CDI storage for something that should be serializable? now the view state is split in two: one managed by jsf, the other in some cdi bag. I can see it could work, but we need to do some tests first to be sure how to deal with this part. #3 converter/validator is StateHolder/PartialStateHolder That's not necessary true. A converter validator can be just a normal class, serializable, stateholder or partialstateholder. The code that deals with that logic is in UIOutput/UIInput. The real question is which is the default lifecycle for a converter/validator in each case. In theory it should be request scope, but maybe it is less than request scope. But if implements StateHolder it should be view scope. I don't remember issues over the current implementation of view scope. Remember view scope is a passivation capable scope, so all beans should implement Serializable. #4 What will be attached to the component tree? a proxy or the real instance? I have never tried that, so I don't have idea about how the code works in that part. In conclusion, my doubts are more related about the interactions between the state management and cdi and the aplicability of the solution for third party libraries, even if I know the proposed patch could work. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788082#comment-13788082 ] Gerhard Petracek commented on MYFACES-3797: --- @ #1 changing the default behavior of the state handling algorithm that isn't correct. the state-handling of this part saves/restores the class of the artifact. that wasn't changed at all. the previous code is: restoredObject = clazz.newInstance(); and now the class is used for a bean-lookup (if the feature is enabled) and only if there is no bean (or the lookup was skipped because the feature isn't enabled) clazz.newInstance() is used (like before). @performance: isAssignableFrom is fast and we can tweak resolveExternalArtifact for the default case. i thought about an adapter already, but it breaks #getConverter and #getValidators (if we are talking about the same). @ #2 see my updated comment. @cdi storage once #2a of my previous comment works, we need the storage-approach for the cleanup (esp. for @PostConstruct methods) of dependent-scoped beans which are serializable. the alternative is a full tree-visit, which is way slower. @ #3: yes and it works. in case of normal-scoped beans the cdi-container handles the lifecycle and in case of dependent beans we remove them at the end of the request (as it is done implicitly for non-cdi converters/validators). no - StateHolder doesn't require them to be view scoped, because instead of creating a new instance manually (that's what we have now), the cdi-container will provide a new instance in case of dependent scoped converters/validators (or the instance managed by the cdi-container in case of normal-scoped beans) and in all those cases the state gets restored the same way (via #restoreState). @view-scope: once #2a is fixed, i'll show it with an example. (@passivation capable scope yes and we don't have an issue here at all) @ #4 yes - you will get a std. cdi-proxy for normal-scoped beans. maybe that's the point you need to know to understand the approach. @ 3rd party libs. i know what you mean, however, with this patch we can fix all issues all other (known) approaches have. if 3rd party libs use their own implementation for state-handling, they are responsible to handle state correctly. @tests: +1 i did them manually and all cases worked so far (the only exception is #2a of the previous comment due to an issue with that part of our current state-saving - i just can test my local addition once serializable converters/validators get restored as well. before that we don't have the issue at all.) cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788108#comment-13788108 ] Dora Rajappan commented on MYFACES-3797: From core perspective [~lu4242] doubts are valid comprehensive. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788113#comment-13788113 ] Gerhard Petracek commented on MYFACES-3797: --- no - as mentioned in my previous comment everything which was mentioned is tested (locally) and works without issues. if you have a case in mind which doesn't work, you are welcome to provide it. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788118#comment-13788118 ] Leonardo Uribe commented on MYFACES-3797: - #1 That's precisely the problem. Before do the instantiation we need to check if cdi is active, then if the feature is enabled, then invoke the method and then we need to multiply that for each converter/validator in the page. What I mean is even if the related code looks fast, in the whole context it is not because that's a sensitive part. About break getConverter() and getValidators() the idea is the adapter implements FacesWrapper. In that way, if the code needs something special (an extra interface in the converter or validator), in that part they can just do the check and get the wrapped instance and that's it. Most of the time that's not necessary, because that's the reason why there is an interface for those objects, to define a contract between the view and the controller. Remember we didn't have a generic FacesWrapper in previous implementations of the spec but now we have it and this is a good example where we can use it. Since we are talking about put this in 2.2 branch, this feature is new and the workaround can be done in third party libraries quite easily (if there is a need, but most of the time it is unlikely) By all previous reasons already exposed, in my opinion a solution using an adapter looks better. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788125#comment-13788125 ] Gerhard Petracek commented on MYFACES-3797: --- it breaks 3rd party libs which check the types of converters and validators. in most cases i've seen so far they don't expect the need of such check, if they created the converters and validators internally. @performance as mentioned before we can tweak the default case (e.g. caching the info if the feature is enabled. it's currently a first draft and not a fully optimized final version). if you enable it, you only have a minor overhead which is smaller than the overhead you get with an external container (you know the numbers). however, if it is the only issue, we can change that part to an adapter in some min., because the rest is the same. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788363#comment-13788363 ] Dora Rajappan commented on MYFACES-3797: Yeah from core perspective specific code in DeltaStateHelper should be avoided and should go for wrapper similar to that of inputFile restore. Rest is excellent. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788406#comment-13788406 ] Gerhard Petracek commented on MYFACES-3797: --- again: the basic algorithm in DeltaStateHelper wasn't changed! we still store the class as we did before. only during the restore the optional lookup is done. if you compare the state before and after the patch, there is no difference in what is stored. with a wrapper, we would add state. if we change the state-handling that only the real class gets stored (instead of the special wrapper) and during the restore the special wrapper gets recreated (with the real class as parameter), we have the logic in two places (instead of one). with that we wouldn't add state, but we still break existing 3rd party libs. the only benefit would be that we support custom state-handling implementations (which isn't mandatory). that means we would improve support for very few libs and break the integration with others (and/or applications). cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788512#comment-13788512 ] Leonardo Uribe commented on MYFACES-3797: - I think the best way to analyze the advantages or disadvantages is to try to forecast the future a bit. Suppose you have a custom input component that does not extend from UIComponentBase but from UIComponent and you need to write the state saving logic. What do you do? you read the spec, and make custom implementations based on the descriptions there. But you haven't realized that small detail done inside UIComponentBase or DeltaStateHelper, that has changed the behavior of a method that has a clear description in the spec javadoc. At the end, your code will not work, because from outside you don't know what's inside myfaces code. And the solution? do some ugly myfaces specific hack that only god knows how to do in your third party code and you'll pray that on further versions it will not get broken. On the other side, the solution using an adapter just introduce a fine layer between the converter validator implementation and the component tree, that will be added only in those cases where CDI injection is required, so by default it is disabled. Using FacesWrapper, we can always get the inner instance. Will the existing code keep working? Yes, of course, remember the wrapper is optional. Now the question is which third party libraries do a type check over the new converter / validator instances that are injected. But more important than that is just using the spec, the third party developer can do the necessary code to inspect the wrapper and check the type correctly. In fact, the code is just trivial. Do we care about existing 3rd party libs for a new feature to be added in MyFaces 2.2.x and that by default is disabled? Absolutely no. That's a load that we don't require to carry on. JSF 2.2 is a major release, it is reasonable to write 2.2 versions of the libraries affected, but remember FacesWrapper is there since 2.0, so the necessary code can be added in 2.0 branches as well. In this case, we have a solution that just breaks encapsulation principle (which means you don't need to know what's inside the code to make it work together) against another (using an adapter) that does not. Now the state problem. To be clear, with a wrapper we don't add state. How is that possible? You can make the wrapper implement PartialStateHolder and with that the problem is solved. I know that, because there are some examples inside MyFaces Core that do precisely that. Write the wrapper can be tricky, but it will work, no doubt about that in my mind. The solution using the adapter could be discussed and included in the spec. Inclusive, we can make an SPI interface for this one. What's the deal about the adapter solution? there is no problem, it is feasible and its drawbacks are minimal compared with the alternative. cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797_2.patch, MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
[jira] [Commented] (MYFACES-3797) cdi support for converters and validators
[ https://issues.apache.org/jira/browse/MYFACES-3797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13788581#comment-13788581 ] Gerhard Petracek commented on MYFACES-3797: --- @custom state-saving: esp. for the upcoming version of the spec. there are ways to support it as well, however, i didn't disagree for the current version of the spec. i just said that it isn't mandatory to support it (for a jsf 2.2 impl.). @only in those cases where CDI injection is required: if you mean that you would like to check the classes in detail: you can't do that easily. you would lose many cdi features like @Alternative, Specializes, @Veto (v1.1),... @libs which do such checks: i know at least one proprietary component lib and at least 2-3 applications, but i agree for sure that it's easy to fix them. @PartialStateHolder agreed, however, i was talking about full state-saving for sure. if we are using a PartialStateHolder, we have to document this restriction. @spec: i haven't said that an adapter isn't easier for the spec., however, only if it is added to the spec. and all libs are compatible with it, we can be sure that it works (with partial-state-saving only). however, since we also need a patch for the spec.-discussion and it's just a small change for the current patch, i'll do it in any case and we can use it for now. (if jsf 2.3 will specify a different approach, we can still change it.) cdi support for converters and validators - Key: MYFACES-3797 URL: https://issues.apache.org/jira/browse/MYFACES-3797 Project: MyFaces Core Issue Type: New Feature Components: JSR-344 Reporter: Gerhard Petracek Assignee: Gerhard Petracek Attachments: MYFACES-3797_2.patch, MYFACES-3797.patch with context-param param-nameorg.apache.myfaces.CONVERTER_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param and context-param param-nameorg.apache.myfaces.VALIDATOR_INJECTION_ENABLED/param-name param-valuetrue/param-value /context-param it should be possible to enable cdi support for converters/validators. we need the config, because it was postponed for the spec. -- This message was sent by Atlassian JIRA (v6.1#6144)
