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

Fixed StrategyInterface::hydrate() docblock because it is not optional #23

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

svycka
Copy link
Contributor

@svycka svycka commented Aug 19, 2020

Q A
Documentation yes

@svycka svycka changed the base branch from develop to master August 19, 2020 12:53
@svycka svycka changed the base branch from master to develop August 19, 2020 12:54
@froschdesign
Copy link
Member

A bigger problem is included in the documentation: https://docs.laminas.dev/laminas-hydrator/v3/strategy/#introduction

@froschdesign froschdesign added the Bug Something isn't working label Aug 19, 2020
@svycka
Copy link
Contributor Author

svycka commented Aug 19, 2020

whops wrong branch bet I see documentation issue so will leave as it is as I don't know how you will address this good luck

@geerteltink geerteltink changed the base branch from develop to 3.2.x September 11, 2020 13:07
Copy link
Contributor

@settermjd settermjd left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for making the change, @svycka. One final step required, https://github.com/laminas/laminas-hydrator/pull/23/checks?check_run_id=1003015316.

@svycka
Copy link
Contributor Author

svycka commented Sep 23, 2020

@settermjd I have fixed it. But I think this is not a docblock bug but interface should allow optional context. I think @weierophinney introduced this unintentionally when added typehints. Also as @froschdesign noted documentation has it correctly https://docs.laminas.dev/laminas-hydrator/v3/strategy/#introduction but changing interface now I am not sure it's BC break or not?

@weierophinney what do you think?

@froschdesign
Copy link
Member

The title of this pull request is to the update the DocBlock and this must be done in the related PHP file and documentation file.

Everything else needs a new issue report or a discussion in the forum.

@weierophinney weierophinney changed the base branch from 3.2.x to 3.1.x October 6, 2020 14:39
@weierophinney weierophinney added this to the 3.1.1 milestone Oct 6, 2020
Signed-off-by: Vytautas Stankus <svycka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants