Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Apr 18, 2019

Else this does 💥

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl

@rullzer rullzer added bug 3. to review Waiting for reviews labels Apr 18, 2019
@rullzer rullzer added this to the Nextcloud 17 milestone Apr 18, 2019
@rullzer
Copy link
Member Author

rullzer commented Apr 18, 2019

/backport to stable16

@kesselb
Copy link
Contributor

kesselb commented Apr 18, 2019

class AccessToken extends Entity {

I think we should fix the getter otherwise we have to cast it on every occurrence.

@nickvergessen
Copy link
Member

Well actually

$this->addType('client_id', 'int');
should do exactly that.

@nickvergessen
Copy link
Member

So I would fix

if(property_exists($this, $name)){
return $this->$name;
} else {
instead.

@kesselb
Copy link
Contributor

kesselb commented Apr 18, 2019

Any idea what is broken? If the record is populated from DB the values should be set through the setters. We could migrate to QBMapper for 17.

@rullzer
Copy link
Member Author

rullzer commented Apr 18, 2019

I have no idea why it does 💥 but it was reported and did 💥 for me on a 16 test with php 7.1 (no xdebug there).

I propose to just get this in and fix it properly later.

@kesselb
Copy link
Contributor

kesselb commented Apr 18, 2019

Sure. I still vote for a getter in accesstoken and do the type cast there.

@rullzer
Copy link
Member Author

rullzer commented Apr 18, 2019

Ah sure that is fine as well

@rullzer rullzer force-pushed the fix/oauth2_string_to_int branch from 97321f6 to 21d542f Compare April 18, 2019 19:10
@rullzer
Copy link
Member Author

rullzer commented Apr 18, 2019

Ah. so it was actually because the wront names were used for setting the types 🙈

Else this does 💥

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

🙈

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

You found the real mistake 🚀

@rullzer rullzer merged commit 5c6deb1 into master Apr 24, 2019
@rullzer rullzer deleted the fix/oauth2_string_to_int branch April 24, 2019 13:45
@backportbot-nextcloud
Copy link

backport to stable16 in #15215

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

Labels

3. to review Waiting for reviews bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants