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

Remove ValueValidator::setOptions from the interface #24

Merged
merged 1 commit into from
Sep 28, 2017

Conversation

thiemowmde
Copy link
Contributor

The one and only reason why this method is in the interface can be seen in DataValues/Validators#11. With the change I'm doing in DataValues Validators the method is not called any more. Which means we can do a very convenient update from "data-values/interfaces": "~0.2.0" to "data-values/interfaces": "~0.3.0|~0.2.0" in that and all other depending components.

@JeroenDeDauw
Copy link
Member

This method is used by the code for which the interface was originally created: https://packagist.org/packages/param-processor/param-processor

I think the interface design is not very good, and that this method makes no sense whatsoever for the Wikibase usecase. The later suggests to not use this interface for that usecase.

If you change this interface here, then you also need to update the code in ParamProcessor, else you're breaking things.

@thiemowmde thiemowmde changed the title Remove ValueValidator::setOptions from the interface [DNM] Remove ValueValidator::setOptions from the interface Feb 15, 2016
@thiemowmde
Copy link
Contributor Author

Used in 2 places in ParamProcessor, both marked with TODOs. Hm. I will have a closer look and try to rework this, but probably not within the next days. Marked as DNM for now.

@bekh6ex
Copy link

bekh6ex commented Feb 3, 2017

Removing method from interface is backwards compatible change in PHP, so no code will break right now, although code won't work with new implementations.
Also, version 0.x means that interfaces can change, so it is also fine.
And in the end, this changes won't affect ParamProcessor as I understood from code, or will they?

So, I think this PR can be merged.

@JeroenDeDauw
Copy link
Member

Removing method from interface is backwards compatible change in PHP, so no code will break right now, although code won't work with new implementations.

Hmm indeed. I wonder if nothing will break though. For instance PHPUnit mocks... would be good to check.

While there might be no runtime error, the interface will no longer be providing what is expected, and you'll end up with a bunch of calls to "undefined" methods. That is not nice.

Also, version 0.x means that interfaces can change, so it is also fine.

To me this is entirely orthogonal to the issue at hand. 0.x range just means you don't need to increment the primary number for breaking changes. So you can go from 0.5 to 0.6 instead of 1.5 to 2.0. That's the difference here. A breaking change is still a breaking change. Ideally 0.1 also implies that the thing is still in early dev and breaking changes will be common. Unfortunately we did not bump the number for this component to 1.0 as we should have long ago, so that is not the reality we now have.

@JeroenDeDauw
Copy link
Member

I think both main users of this interface will be better of if they use their own one. Sure, you duplicate 5 lines over two projects, but the cost of that is nothing compared to the added coupling this thing causes.

@bekh6ex
Copy link

bekh6ex commented Feb 3, 2017

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

From http://semver.org/

@JeroenDeDauw
Copy link
Member

I'm well aware of that. See the last lines of my comment.

@thiemowmde thiemowmde changed the title [DNM] Remove ValueValidator::setOptions from the interface Remove ValueValidator::setOptions from the interface Mar 6, 2017
@thiemowmde
Copy link
Contributor Author

I had a look at the ParamProcessor repository, and one of the two usages assumes specific implementations anyway by checking for a method named "getWhitelistedValues". The other looks like the options can easily be injected into the definition object while creating the specific ValueValidator, instead of passing the options around as a separate field.

I suggest to merge DataValues/Validators#11 and this pull request, do releases and update code that needs to be updated later. No code will break immediately as long as no relevant implementation starts removing the setOptions method. I will happily help updating that ParamProcessor repository if necessary.

@JeroenDeDauw
Copy link
Member

If you commit to doing the work caused by this change in a timely manner than I'm all for it.

@JeroenDeDauw
Copy link
Member

Thanks for JeroenDeDauw/ParamProcessor#29

@JeroenDeDauw JeroenDeDauw merged commit 592efd0 into master Sep 28, 2017
@JeroenDeDauw JeroenDeDauw deleted the setOptions branch September 28, 2017 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants