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

Use link extractors #103

Closed
wants to merge 5 commits into from
Closed

Use link extractors #103

wants to merge 5 commits into from

Conversation

neeckeloo
Copy link
Contributor

The goal is to start reducing the scope of the plugin by adding some classes with well defined role.

  • Add LinkExtractor and LinkCollectionExtractor
  • Create specific unit test classes for both
  • Add additional unit tests

@Wilt
Copy link
Contributor

Wilt commented Jun 23, 2015

I like this idea. It will also allow people to add their own Link rendering logic by extending the existing classes and overwriting the methods.

@weierophinney
Copy link
Member

@neeckeloo I ❤️ this! Looks like you may need a few more tests? Also, some docs in the README about the new classes and where they fit in would be useful for those that want to use the features in a more advanced fashion.

@neeckeloo
Copy link
Contributor Author

I currently use inheritance to manage the relationship between LinkExtractor and LinkExtractorCollection but aggregation might be more flexible. Is it necessary or not? What do you think about?

@gsomoza
Copy link
Contributor

gsomoza commented Jul 1, 2015

IMHO while inheritance is definitely works, aggregation would have the added benefit that if there are other types of LinkExtractors (or other ExtractorInterface classes) they could still be used by LinkExtractorCollection (i.e. more flexibility, like you said). Otherwise a new collection extractor would be required for each type of LinkExtractor, or so it seems to me right now. Does that make sense?


`LinkExtractor` is responsible for extracting a link representation from `Link` instance.

#### ZF\Hal\Extractor\LinkExtractorCollection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/LinkExtractorCollection/LinkCollectionExtractor/ (I'll do that during merge; just noting it so I don't forget.)

weierophinney added a commit that referenced this pull request Jul 6, 2015
weierophinney added a commit that referenced this pull request Jul 6, 2015
- Removed dependency on `Zend\Stdlib\Extractor\ExtractionInterface`,
  and:

  - Added `extract(Link $link)` to `LinkExtractorInterface`.
  - Added `extract(LinkCollection $collection)` to `LinkCollectionExtractorInterface`.

  In each case, this allowed removing the initial conditional that
  checked for a valid type (as the check is happening prior to invoking
  the method regardless!), and removing an obsolete test for this edge
  case.

- Removed `setLinkExtractor()` from `LinkCollectionExtractorInterface`,
  as it's unused in the given API, and should be an implementation
  detail. It was *not* removed from the implementation.

- Changed `{@inheritdoc}` annotations to `@inheritDoc`.

- Updated copyright to 2015.
weierophinney added a commit that referenced this pull request Jul 6, 2015
weierophinney added a commit that referenced this pull request Jul 6, 2015
@weierophinney
Copy link
Member

Merged to develop for release on the 1.2.0 tag.

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