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

Force _fromSeqenceString result array values to int #6

Conversation

miaulalala
Copy link

as currently, the first value will be a string.

Traced it down to the _fromSequenceString method:

The iteration with ++ will force the second loop to convert to int, but the first loop is using as a string leading to errors with strongly typed methods.

Seen in error logs like this:

  1. /www/htdocs/nextcloud/apps/mail/lib/Service/Sync/ImapToDbSynchronizer.php - line 453:
    OCA\Mail\Db\MessageMapper->deleteByUid(OCA\Mail\Db\Mailbox { id: 23}, "1", 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255, 256, 257, 258, 259, 260, 261, 262, 263, 264, 265, 266, 267, 268, 269, 270, 271, 272, 273, 274, 275, 276, 277, 278, 279, 280, 281, 282, 283, 284, 285, 286, 287, 288, 289, 290, 291, 292, 293, 294, 295, 296, 297, 298, 299, 300, 301, 302, 303, 304, 305, 306, 307, 308, 309, 310, 311, 312, 313, 314, 315, 316, 317, 318, 319, 320, 321, 322, 323, 324, 325, 326, 327, 328, 329, 330, 331, 332, 333, 334, 335, 336, 337, 338, 339, 340, 341, 342, 343, 344, 345, 346, 347, 348, 349, 350, 351, 352, 353, 354, 355, 356, 357, 358, 359, 360, 361, 362, 363, 364, 365, 366, 367, 368, 369, 370, 371, 372, 373, 374, 375, 376, 377, 378, 379, 380, 381, 382, 383, 384, 385, 386, 387, 388, 389, 390, 391, 392, 393, 394, 395, 396, 397, 398, 399, 400, 401, 402, 403, 404, 405, 406, 407, 408, 409, 410, 411, 412, 413, 414, 415, 416, 417, 418, 419, 420, 421, 422, 423, 424, 425, 426, 427, 428, 429, 430, 431, 432, 433, 434, 435, 436, 437, 438, 439, 440, 441, 442, 443, 444, 445, 446, 447, 448, 449, 450, 451, 452, 453, 454, 455, 456, 457, 458, 459, 460, 461, 462, 463, 464, 465, 466, 467, 468, 469, 470, 471, 472, 473, 474, 475, 476, 477, 478, 479, 480, 481, 482, 483, 484, 485, 486, 487, 488, 489, 490, 491, 492, 493, 494, 495, 496, 497, 498, 499, 500)

Signed-off-by: Anna Larch anna@nextcloud.com

@miaulalala miaulalala changed the title Force _fromSeqenceString result array values to ìnt` Force _fromSeqenceString result array values to int Aug 4, 2022
@bytestream
Copy link
Owner

I'm not sure about this. Are ids guaranteed to to be integers?

@miaulalala
Copy link
Author

I'm not sure about this. Are ids guaranteed to to be integers?

That's a good question. The method itself only talks about indices in the comment, but it doesn't specify the type.

RFC 3501 doesn't specify any formats either:

A 32-bit value assigned to each message, which when used with the
unique identifier validity value (see below) forms a 64-bit value
that MUST NOT refer to any other message in the mailbox or any
subsequent mailbox with the same name forever.

I leave it up to you to decide - I personally woud be in favour of a consistent return type, but of cousre theroetically, UIDs could be strings as long as they're iterable - hex values come to mind.

@bytestream
Copy link
Owner

I think (string) is probably the correct cast. The explode() lines will add string values to the $ids list. It's only the for loop that adds a mix of string and int.

If you could add a test that would be awesome... something like the below might work:

$this->assertSame(['1','2','3','4'], (new Ids('1,2,3,4'))->ids);

@miaulalala
Copy link
Author

K so a further googling lead me to page 91 of the RFC. I think the values are indeed meant to be non zero integers, but please correct me if I'm wrong.
The explode() should theoretically also return ints, not strings. Not a problem as long as methods don't use strong types.

https://www.rfc-editor.org/rfc/rfc3501.html#page-91

@bytestream
Copy link
Owner

I believe you're correct, so please correct both cases and add a test

as currently, the first value will be a string. The iteration with ++
will force the second loop to convert  to int, but the first loop
is using  as a string leading to errors with strongly typed methods.

Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala miaulalala force-pushed the fix/force-int-for-sequence-string-conversion branch from b5b1aa8 to 2687882 Compare August 10, 2022 05:44
@miaulalala
Copy link
Author

miaulalala commented Aug 10, 2022

Let's see if the tests pass, unfortunately I have no way to run them locally at the moment.

Also added a forced INT to the add function.

@bytestream bytestream merged commit ceafee1 into bytestream:master Aug 10, 2022
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.

2 participants