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

Moved value setter to constructor #75

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rdotter
Copy link

@rdotter rdotter commented Nov 27, 2018

  • Changed value function into only getter of the value for a generator class
  • Moved setter of __value to constructor (where is should be)

@goetas

@goetas
Copy link
Member

goetas commented Nov 27, 2018

I think this channges are not backward compatible, are they?

@rdotter
Copy link
Author

rdotter commented Nov 27, 2018

@goetas the setter via value is only used by JMS and I hope not ever by a developer :-)
So when you regenerate the models + yml everything keeps working only the generated models differs a bit (less code).

If a developer used the value function for setting this variable for creating model for generating XML it will break because it is no longer accepted by this method. I think its a consistency improvement to do it this way.

@rdotter
Copy link
Author

rdotter commented Nov 30, 2018

@goetas can you merge this PR? :-)

@goetas
Copy link
Member

goetas commented Nov 30, 2018

I'm worried about BC compatibility and more than that, I'm not sure if is a good idea to call __construct on an already initialized object.

@rdotter
Copy link
Author

rdotter commented Nov 30, 2018

@goetas you are totally right, I thought it would call the construct on creation of the class, but I found you have to use an unserialize_object_constructor.class. I'll look into this next week. Thanks for the feedback.

Maybe only the doc change is required for now, I only dislike the hidden setter :-)

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.

3 participants