Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Deprecate hydrator classes #20

Merged
merged 16 commits into from
Sep 21, 2015

Conversation

weierophinney
Copy link
Member

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

The following is a checklist for what needs to happen:

  • Add a dependency on zendframework/zend-hydrator
  • Alter existing classes to extend their zend-hydrator equivalents. In some cases, this may require either decoration (some classes are marked final) or changes to zend-hydrator to accommodate extension.
  • Mark each class as @deprecated; we may also want to raise an E_USER_DEPRECATED error to flag to users they will need to update.
  • The HydratorPluginManager should always return the zend-hydrator instances, not the zend-stdlib equivalents.

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

@Maks3w
Copy link
Member

Maks3w commented Sep 17, 2015

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
Copy link
Member Author

@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
Copy link
Member

Maks3w commented Sep 17, 2015

Is it important to assert both Hydrators are the same?

@weierophinney
Copy link
Member Author

@Maks3w for typehints, yes.

@weierophinney
Copy link
Member Author

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 added a commit to weierophinney/zend-stdlib that referenced this pull request Sep 18, 2015
@weierophinney
Copy link
Member Author

@bakura10 and @ezimuel — please review. 😅

@Maks3w
Copy link
Member

Maks3w commented Sep 18, 2015

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
Copy link
Contributor

ezimuel commented Sep 21, 2015

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

- Updated interfaces, abstract classes, and concrete classes to extend
  zend-hydrator counterparts.
- Where required, also implemented local hydrator interfaces to allow extensions
  to comply.
- Most test modifications were around updating exception expectations to look
  for the zend-hydrator equivalents.
- Each also indicates the class from zendframework/zend-hydrator to use.
- unused use statements
This was needed to ensure that objects typehinting on
`Zend\Stdlib\Extractor\ExtractionInterface` specifically would continue to work
with objects implementing HydratorInterface. (Discovered in testing Apigility;
this one change makes Apigility work correctly.)
- In case any code is type-hinting on `HydrationInterface` from
  zend-stdlib, this ensures objects fulfilling `HydratorInterface` will
  fulfill those as well.
@weierophinney
Copy link
Member Author

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.

Now presents notes on how to update code to work with the removal of
the Hydrator classes.
@weierophinney
Copy link
Member Author

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.

Before this patch, if a typehint was on
`Zend\Stdlib\Hydrator\HydratorInterface`, and the hydrator extend the
`AbstractHydrator`, then the type check would fail, as it was not
implementing the interface.
@weierophinney
Copy link
Member Author

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
Copy link
Member Author

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.

@mwillbanks mwillbanks merged commit 77d7dbe into zendframework:develop Sep 21, 2015
mwillbanks added a commit that referenced this pull request Sep 21, 2015
mwillbanks added a commit that referenced this pull request Sep 21, 2015
@weierophinney weierophinney deleted the feature/20 branch September 21, 2015 21:43
@weierophinney
Copy link
Member Author

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:

  • zend-modulemanager (Feature\HydratorProviderInterface)
  • zend-form (Factory, FieldsetInterface, Fieldset, Form, Annotation\FormAnnotationsListener, Annotation\AnnotationBuilder, Annotation\Hydrator, Annotation\ElementAnnotationsListener)
  • zend-mvc (Service\HydratorManagerFactory, Service\ServiceListenerFactory, Service\ModuleManagerFactory)
  • zend-db (ResultSet\HydratingResultSet)
  • zf-hal (Plugin\Hal)
  • zf-apigility-admin

Checklist:

  • New maintenance release
    • Update composer.json to reference stdlib < 2.7
    • Open PR and test on travis
    • Tag new release
  • New minor release
    • Update develop branch to require zend-hydrator >= 2.0
    • Update develop branch to require zend-stdlib >= 2.7
    • Update code to reference zend-hydrator interfaces and/or concrete instances
    • Open PR and test on travis
    • Merge to master
    • Tag new minor release

weierophinney added a commit to weierophinney/zend-stdlib that referenced this pull request Oct 14, 2015
Per zendframework#20, this removes the hydrator functionality from zend-stdlib.
weierophinney added a commit to weierophinney/zend-stdlib that referenced this pull request Oct 15, 2015
Per zendframework#20, this removes the hydrator functionality from zend-stdlib.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants