Skip to content

Fix userid casting in notifications - #15912

Merged
skjnldsv merged 1 commit into
masterfrom
fix/notifications/user-id-casting
Jun 11, 2019
Merged

Fix userid casting in notifications#15912
skjnldsv merged 1 commit into
masterfrom
fix/notifications/user-id-casting

Conversation

@skjnldsv

@skjnldsvskjnldsv commented Jun 11, 2019

Copy link
Copy Markdown
Member

@skjnldsvskjnldsv self-assigned this Jun 11, 2019
@skjnldsvskjnldsv added this to the Nextcloud 17 milestone Jun 11, 2019
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

/backport to stable16

Comment threadlib/private/Notification/Notification.php Outdated
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

@ChristophWurst this is what we should do right?
use \string to force php to not check for existing string class?

Because tests are failing
PHP Fatal error: Scalar type declaration 'string' must be unqualified in /drone/src/lib/private/Notification/Notification.php on line 136

@skjnldsv
skjnldsvforce-pushed the fix/notifications/user-id-casting branch from 1afcef5 to 490d1f0CompareJune 11, 2019 13:38
@ChristophWurst

Copy link
Copy Markdown
Member

@ChristophWurst this is what we should do right?

Not for primitive types. Only for built-in functions like \count it does make sense.

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Yep, I just realised that ^^
Fixed!

@skjnldsvskjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 11, 2019
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
skjnldsvforce-pushed the fix/notifications/user-id-casting branch from 490d1f0 to 49d5030CompareJune 11, 2019 14:53
@skjnldsv
skjnldsv merged commit cf2d123 into masterJun 11, 2019
@skjnldsv
skjnldsv deleted the fix/notifications/user-id-casting branch June 11, 2019 16:30
@backportbot-nextcloud

Copy link
Copy Markdown

backport to stable16 in #15925

@kesselb

Copy link
Copy Markdown
Contributor

cc @corecode

@corecode

Copy link
Copy Markdown

thanks!

@juliusknorr

Copy link
Copy Markdown
Member

@skjnldsv Tests are still failing due to this on master. Mind to have a look?


1) Test\Notification\NotificationTest::testSetAppInvalid with data set #0 (true)
--
152 | Failed asserting that exception of type "\InvalidArgumentException" is thrown.
153 | 154 | 2) Test\Notification\NotificationTest::testSetAppInvalid with data set #2 (0)
155 | Failed asserting that exception of type "\InvalidArgumentException" is thrown.
156 | 157 | 3) Test\Notification\NotificationTest::testSetAppInvalid with data set #3 (1)
158 | Failed asserting that exception of type "\InvalidArgumentException" is thrown.
159 | 160 | 4) Test\Notification\NotificationTest::testSetAppInvalid with data set #5 (array())
161 | Failed asserting that exception of type "TypeError" matches expected exception "\InvalidArgumentException". Message was: "Argument 1 passed to OC\Notification\Notification::setApp() must be of the type string, array given, called in /drone/src/tests/lib/Notification/NotificationTest.php on line 108" at
162 | /drone/src/lib/private/Notification/Notification.php:137
163 | /drone/src/tests/lib/Notification/NotificationTest.php:108
164 | .
165 | 166 | 5) Test\Notification\NotificationTest::testSetAppInvalid with data set #7 (array('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'))
167 | Failed asserting that exception of type "TypeError" matches expected exception "\InvalidArgumentException". Message was: "Argument 1 passed to OC\Notification\Notification::setApp() must be of the type string, array given, called in /drone/src/tests/lib/Notification/NotificationTest.php on line 108" at
168 | /drone/src/lib/private/Notification/Notification.php:137
169 | /drone/src/tests/lib/Notification/NotificationTest.php:108
170 | .
171 | 172 | 6) Test\Notification\NotificationTest::testSetUserInvalid with data set #0 (true)
173 | Failed asserting that exception of type "\InvalidArgumentException" is thrown.
174 | 175 | 7) Test\Notification\NotificationTest::testSetUserInvalid with data set #2 (0)
176 | Failed asserting that exception of type "\InvalidArgumentException" is thrown.
177 | 178 | 8) Test\Notification\NotificationTest::testSetUserInvalid with data set #3 (1)
179 | Failed asserting that exception of type "\InvalidArgumentException" is thrown.
180 | 181 | 9) Test\Notification\NotificationTest::testSetUserInvalid with data set #5 (array())
182 | Failed asserting that exception of type "TypeError" matches expected exception "\InvalidArgumentException". Message was: "Argument 1 passed to OC\Notification\Notification::setUser() must be of the type string, array given, called in /drone/src/tests/lib/Notification/NotificationTest.php on line 136" at
183 | /drone/src/lib/private/Notification/Notification.php:159
184 | /drone/src/tests/lib/Notification/NotificationTest.php:136
185 | .
186 | 187 | 10) Test\Notification\NotificationTest::testSetUserInvalid with data set #7 (array('aaaaaaaaaaaaaaaaaaaaaaaaaaaaa...aaaaaa'))
188 | Failed asserting that exception of type "TypeError" matches expected exception "\InvalidArgumentException". Message was: "Argument 1 passed to OC\Notification\Notification::setUser() must be of the type string, array given, called in /drone/src/tests/lib/Notification/NotificationTest.php on line 136" at
189 | /drone/src/lib/private/Notification/Notification.php:159
190 | /drone/src/tests/lib/Notification/NotificationTest.php:136
191 | .

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Ah damn, I missed those!
Thanks!

@nickvergessen

Copy link
Copy Markdown
Member

the type hint is also part of #15040

Also I would suggest to revert the backport of the type hint and only keep the cast in 16, which also already fixes the problem.

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

@nickvergessen sure thing!
Do whatever you need :)

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

Labels

4. to releaseReady to be released and/or waiting for tests to finishbug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InvalidArgumentException","Message":"The given user id is invalid

6 participants

@skjnldsv@ChristophWurst@kesselb@corecode@juliusknorr@nickvergessen