Add comments expire date

[vitormattos]

Related: nextcloud/spreed#7327

[nickvergessen]

Tested with our instance:

SELECT COUNT(*) FROM oc_comments;
+----------+
| COUNT(*) |
+----------+
|  1059368 |
+----------+

ALTER TABLE `oc_comments` ADD `expire_date` DATETIME NULL AFTER `reactions`; 
Query OK, 0 rows affected (0.028 sec)
Records: 0  Duplicates: 0  Warnings: 0

ALTER TABLE `oc_comments` ADD INDEX `expire_date` (`expire_date`); 
Query OK, 0 rows affected (2.084 sec)
Records: 0  Duplicates: 0  Warnings: 0

So sounds good.

[vitormattos]

Now, can I squash the commits?

[nickvergessen]

Now, can I squash the commits?

Sounds good to me

[nickvergessen]

you need to run bash build/autoload... and commit the changed files in lib/composer/composer/autoload_* (not the installed.php and InstalledVersion.php)

[vitormattos]

Autoload updated with new migration file and commits squashed.

[blizzz]

CI is unhappy

expire_date is also not exposed in DAV API, making it inconsistent with the other features. Cf. apps/dav/lib/Comments/CommentNode.php

[blizzz]

Suggested change

	$comment = $manager->get(strval($ids[1]));
	$comment = $manager->get((string)$ids[1]);

micro optimization :)

[vitormattos]

Good question. I tested the two ways and don't identify the difference. Can you explain the optimization in this case?
https://gist.github.com/vitormattos/e242d41f92dc1ae4548c7311644fb287

[vitormattos]

PS: I only followed the pattern to get string value used in other methods of this test class, but normally I prefer to use (string) only because I think is most common to found in texts.

Only by curiosity I did this:

git grep strval|wc -l
32
git grep "(string)"|wc -l
481

Maybe, I think is good to change the 32 places of strval in server repository to (string)

[vitormattos]

I did other test to check the opcode in site 3v4l.org using the follow code:

echo (string) 1;
echo strval(1);
echo (string) 1;

...and the opcode is the same:

line      #* E I O op                           fetch          ext  return  operands
-------------------------------------------------------------------------------------
    3     0  E >   CAST                                          6  ~0      1
          1        ECHO                                                     ~0
    4     2        CAST                                          6  ~1      1
          3        ECHO                                                     ~1
    5     4        CAST                                          6  ~2      1
          5        ECHO                                                     ~2
    6     6      > RETURN                                                   1

https://3v4l.org/kiXe0/vld

I duplicated the first row to check if the return is from strval or is the return of script, the return is from script.

[blizzz]

Ok, it was intval vs int-casting. Never mind then!

[nickvergessen]

expire_date is also not exposed in DAV API

Neither is the optional refernce_id that we added for talk specifically. I think that's fine.

[blizzz]

expire_date is also not exposed in DAV API

Neither is the optional refernce_id that we added for talk specifically. I think that's fine.

Does not mean it is not worth to add it there as well. The reference id has of course less meaning for file comments. The expiry date would be more relevant, but on the other hand the web ui does not offer this option either. Thus, I also agree it is okay to leave it out, nevertheless worth to point out that this is done deliberately.

P.S.: i.e. my rejection is about the failing tests.

[nickvergessen]

Missing a version bump