zendframework / zend-stdlib

Stdlib component from Zend Framework
BSD 3-Clause "New" or "Revised" License
384 stars 76 forks source link

Deprecate hydrator classes #20

Closed weierophinney closed 9 years ago

weierophinney commented 9 years ago

As zend-hydrator has now been created, we can deprecate the various Hydrator classes.

The following is a checklist for what needs to happen:

Once complete, we will also need to update zend-mvc to start using zend-hydrator for the hydrator plugin manager integration.

Maks3w commented 9 years ago

For avoid the BC Break I suggest one of this:

  1. Remove the final keyword
  2. Move the content of the final class to a parent class and extend from their plus add a gigant message about not extend from that place.
  3. Maintain a copy of that hydrator class and just deprecate.

I think the option 3 it's better. Just maintain a copy of that hydrator on this repo.

weierophinney commented 9 years ago

@Maks3w Option 3 would be simplest; the problem is that you wouldn't be able to assert that it's an instance of the original class from zend-hydrator — which is where we'd need to remove the final keyword. This could be done in v1 of that component, and v2 could re-instate the final keyword; zend-stdlib would pin to v1 until we remove the hydrators.

Maks3w commented 9 years ago

Is it important to assert both Hydrators are the same?

weierophinney commented 9 years ago

@Maks3w for typehints, yes.

weierophinney commented 9 years ago

Test coverage changes are to be expected with code removal. Honestly, looking at the report, it looks like it's a bunch of small coverage changes that, per file, are less than 1%, but which aggregate. I'm not concerned about it; tests are still passing.

weierophinney commented 9 years ago

@bakura10 and @ezimuel — please review. :sweat_smile:

Maks3w commented 9 years ago

Since v2.2 of phpunit coverage @deprecate has the same meaning of @coverageignore for that reason the column of "relevant lines" drops to 0 and a coverage of 100% is returned.

So because the project now has less "relevant lines" now each line has more weight in the global percent and uncovered lines got more relevance.

ezimuel commented 9 years ago

@weierophinney I reviewed the PR and it looks good to me.

weierophinney commented 9 years ago

I've tested Apigility against the current stable develop branch. 7752b57 fixes the only issue I encountered, which is that zf-hal was doing some type-hinting against Zend\Stdlib\Extractor\ExtractionInterface, and thus objects that implemented Zend\Hydrator\HydratorInterface were failing that contract. Changing Zend\Stdlib\Hydrator\HydratorInterface to extend Zend\Stdlib\Extractor\ExtractionInterface solved the issue, while still allowing such implementations to fulfill the zend-hydrator interfaces. (I mirrored this in 2e36707 to ensure any typehints against Zend\Stdlib\Hydrator\HydrationInterface when compared to objects implementing HydratorInterface will continue to work, too.)

At this point, the only thing that will require a change are any objects implementing HydratorAwareInterface, as they will need to update their signatures to use the zend-hydrator interfaces in the argument typehints.

weierophinney commented 9 years ago

I've updated the changelog to detail any necessary changes users may need. If using HydratorAwareTrait, no changes are required; for those implementing HydratorAwareInterface on their own, they will need to update the function signature of setHydrator(). Otherwise, everything is now working exactly as it was before, just with a different inheritance tree.

weierophinney commented 9 years ago

In IRC, @mwillbanks noted an issue in zend-db's HydratingResultSet when using a custom hydrator extending AbstractHydrator; HydratingResultSet typehints on Zend\Stdlib\Hydrator\HydratorInterface, and the custom hydrator was failing the type check.

I've now updated AbstractHydrator in zend-stdlib to also implement the local HydratorInterface, and added a test to demonstrate the problem (as well as the resolution).

weierophinney commented 9 years ago

To satisfy the LSP, I've updated all concrete classes to not only extend the relevant zend-hydrator counterparts, but to also implement the relevant local interfaces. This should obviate any issues with substitution in other classes.

One note at this time: users adopting zend-hydrator and implementing those interfaces will be in a strange situation where that code will not work on code targeting the stdlib interfaces. I'll note that in the release notes.

weierophinney commented 9 years ago

As a follow-up, I'll be updating the following repositories, using the following checklist, in the coming days to ensure they will work going forward, and use the zend-hydrator component for new minor versions:

Checklist: