Skip to content

Conversation

@dartcafe
Copy link

@dartcafe dartcafe commented Jun 25, 2021

fix #642

added Member::getType() as well

@dartcafe dartcafe requested a review from ArtificialOwl June 25, 2021 22:16
@ArtificialOwl
Copy link
Member

I see no problem to re-implement the deprecated getType() on Member, but what would be the purpose of getTypeLongString() as Circles are not defined by Type anymore ?

@dartcafe
Copy link
Author

dartcafe commented Jun 26, 2021

I see no problem to re-implement the deprecated getType() on Member, but what would be the purpose of getTypeLongString() as Circles are not defined by Type anymore ?
Ehm, yeah. Seems I confused myself because I used the TypeLongString() as description and so I confounded it with the new description.

Short story: You're right, it is nonsense.

@dartcafe dartcafe changed the title keep deprecated Circles::gettypeLongString keep some deprecated Methods Jun 26, 2021
@dartcafe
Copy link
Author

I added some mappings from the new types to the deprecated in Member::getType().

It seems that the compatibilty can be secured this way. At least I tested it successful and I can resolve the circles into their members.

@ArtificialOwl
Copy link
Member

Not a huge fan of:

  • having DeprecatedMember included, please use hard coded value.
  • having getType() returning a different value than getUserType(). This will bring more problems than it should fix. If you really want the old value, please create a new getDeprecatedType() or getOldType() that convert the new type value to the old one (and hard code the returned value in the method).

@dartcafe dartcafe requested a review from ArtificialOwl June 27, 2021 07:48
@dartcafe
Copy link
Author

dartcafe commented Jun 27, 2021

having getType() returning a different value than getUserType(). This will bring more problems than it should fix. If you really want the old value, please create a new getDeprecatedType() or getOldType() that convert the new type value to the old one (and hard code the returned value in the method).

This won't help, as I have to get polls compatible between NC21 (circles 0.21) and NC22 (Circles 22.0). And I am not able to release a NC22 and NC21 version. Otherwise we would have to remove the Circles support until the API is stable over more than one version.

With the deprecated function getType() with returning types compatible to the old keys, the meaning is still consistent if the constants are used.

@ArtificialOwl
Copy link
Member

If you do not store the value in your database, you can already have the correct correspondence using \OCA\Circles\Model\Member::TYPE_* in nc21 and nc22. It will returns the correct value depends on the version of Circles installed.

If you store the value, I can add a getDeprecatedtype() in both version.

What do you think ?

@dartcafe
Copy link
Author

dartcafe commented Jun 27, 2021

If you do not store the value in your database, you can already have the correct correspondence using \OCA\Circles\Model\Member::TYPE_* in nc21 and nc22.

But not when using OCA\Circles\Api\v1::TYPE_*. And that is, what we do, because we rely on an unchanged and consistent API.

If we would store the numeric value of the circle member type (which we don't), the problem would be much bigger, because all stored mail members would become a contact member, without notification and a migration based on the installed Circles version would be neccessary.

I got aware of the changes in Circles just by fortune, because a test broke, and I noticed the new implementation in NC22.

To understand my problem:

Circles 0.21

  • OCA\Circles\Api\v1::TYPE_MAIL(3) = OCA\Circles\Model\Member::TYPE_MAIL(3)
  • OCA\Circles\Api\v1::TYPE_CONTACT(4) = OCA\Circles\Model\Member::TYPE_CONTACT(4)

In Circles 22

  • OCA\Circles\Api\v1::TYPE_MAIL(3) != OCA\Circles\Model\Member::TYPE_MAIL(4)
  • OCA\Circles\Api\v1::TYPE_CONTACT(4) != OCA\Circles\Model\Member::TYPE_CONTACT(8)

If we leave the deprecated Member::getType() and return also the deprecated values, the API can be used consitently.

Or you could get rid of the deprecated types and make sure, that OCA\Circles\Api\v1::TYPE_* is identically to OCA\Circles\Model\Member::TYPE_*.

I am not sure, which apps (not maintained by Nextcloud) integrate circles. But there is a risk, that the devolpers of these apps will get surprised by the changes, too. Also this change should be added to the critical changes info in the nextcloud repo.

If it helps to understand, here is the affected code of polls:
https://github.com/nextcloud/polls/blob/48b3eca4bca1186ed5e413d5272fe945dde355b5/lib/Model/Circle.php#L73-L92

If you store the value, I can add a getDeprecatedtype() in both version.

Since AFAIK we cannot define dependencies to other apps, this won't help.

@ArtificialOwl
Copy link
Member

tell me if #648 helps you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[v22.0.0-beta.1] Keep Circle::gettypeLongString() as deprecated method

3 participants