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

API breakage in return type of LinkTracking() #155

Closed
sminnee opened this issue May 25, 2018 · 6 comments
Closed

API breakage in return type of LinkTracking() #155

sminnee opened this issue May 25, 2018 · 6 comments

Comments

@sminnee
Copy link
Member

sminnee commented May 25, 2018

First raised in silverstripe/recipe-cms#12 and discussed during its introduction at #75 (comment)

LinkTracking() now returns SiteTreeLink objects when it used to return SiteTree objects.

It should be corrected to return SiteTree objects, and if necessary a new method can be created to return SiteTreeLink.

Otherwise it's an API breakage.

We should fix this before 4.2 is made stable so that people don't inadvertently start correcting their code to the new API.

Marking as critical as it's a release blocker.

@sminnee
Copy link
Member Author

sminnee commented May 25, 2018

Please note that marketing versioned as 2.0 would really imply that the recipe is 5.0, which is unacceptable in terms of getting people actually using our updated code. Unlike the GraphQL module we can't make the argument that versioned is an "experimental" API.

@tractorcow tractorcow self-assigned this Jun 6, 2018
@tractorcow
Copy link
Contributor

tractorcow commented Jun 6, 2018

Back from China and able to properly look at this.

LinkTracking() now returns SiteTreeLink objects when it used to return SiteTree objects.

Actually the change is that it used to return a ManyManyList but now returns a ManyManyThroughList, which is another sibling class. Both extend RelationList base.

Both lists dataClass() are SiteTree, and has not been changed to SiteTreeLink.

@tractorcow
Copy link
Contributor

See my comments at silverstripe/recipe-cms#12 (comment) also.

@tractorcow
Copy link
Contributor

@sminnee I'm changing my mind about this. Can you please read my updates (here and on the cms recipe ticket) and let me know if you agree. I don't think this is a real bug.

@sminnee
Copy link
Member Author

sminnee commented Jun 6, 2018

Hopefully we can get some confirmation about the other bug. Seems reasonable, thouhg.

Perhaps we should update the docblock on LinkTracking to clarity what return type we are committing to (RelationList?). Is it reasonable to expect callers to only rely on facilities exposed by RelationList?

@chillu
Copy link
Member

chillu commented Jun 11, 2018

Related phpstan issue has been closed (https://github.com/silbinarywolf/silverstripe-phpstan/issues/13), according to Damian this isn't a bug. Closing.

@chillu chillu closed this as completed Jun 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants