Remove type hinting from timestamps that slack returns as both integer and string

[pyrech]

Hi, thanks for your contribution :slightly_smiling_face:

The files you edited are automatically generated from the OpenAPI specification so they should not be be modified manually. Instead you should update the SDK by patching the JSON spec. See our documentation on how to achieve that :wink: Thanks

[lsnider]

I'm sorry, didn't mean to make this a PR yet. This is still a WIP. Should have this done/cleaned up today

[lsnider]

There was an effort in recent PRs to move all timestamps from integers to strings. We found three more timestamps set as integers that were breaking on some old Slack messages circa 2014. When we changed the type to string though, the library started breaking on new Slack messages because Slack was returning newer message timestamps as integers.

In summary, for these fields, we've seen Slack return both string and integers and neither type would work for all messages. This PR removes the type hinting for the three fields we found to get returned both ways.

[lsnider]

We checked the failing CI tests. The tests failing look to not be related. We see them failing in a previous PR too. Happy to make any changes you'd like to see though.

[lsnider]

Wanted to send a quick followup. We are currently running production on this PR and it has cleaned up all remaining issues we've seen thus far.

[damienalexandre]

Hi there.

Sorry this PR did not make it sooner into the library. The fact that we cannot reproduce is indeed one of the reasons.

But let's move forward. We have a new procedure to update the SDK / add a patch: https://github.com/jolicode/slack-php-api/blob/master/doc/updating-sdk.md#generating-a-new-patch

Would you be willing to give it a try or should we do it?

Thanks a lot, Cheers

[devsibwarra]

@damienalexandre I tried using just v3.0.0 and was able to capture the failure

JoliCode\Slack\Api\Normalizer\ObjsFileNormalizer::denormalize data: array (
  'id' => 'F01234ABC',
  'created' => 1389889200,
  'timestamp' => '1389889334',
  'name' => 'super_awesome_file',
  'title' => 'Super Awesome File',
  'mimetype' => 'text/plain',
  'filetype' => 'space',
  'pretty_type' => 'Post',
  'user' => 'U01234ABC',
  'editable' => true,
  'size' => 680,
  'mode' => 'space',
  'is_external' => false,
  'external_type' => '',
  'is_public' => true,
  'public_url_shared' => false,
  'display_as_bot' => false,
  'username' => '',
  'url_private' => 'https://files.slack.com/files-pri/T01234ABC-F01234ABC/super_awesome_file',
  'url_private_download' => 'https://files.slack.com/files-pri/T01234ABC-F01234ABC/download/super_awesome_file',
  'permalink' => 'https://workspace.slack.com/files/U01234ABC/F01234ABC/super_awesome_file',
  'permalink_public' => 'https://slack-files.com/T01234ABC-F01234ABC-012345678a',
  'preview' => '<document>...</document>',
  'updated' => '1389889334',
  'editor' => 'U01234ABC',
  'last_editor' => 'U01234ABC',
  'state' => 'locked',
  'is_starred' => false,
  'has_rich_preview' => false,
)

   TypeError

  Argument 1 passed to JoliCode\Slack\Api\Model\ObjsFile::setTimestamp() must be of the type int or null, string given, called in /repo/cloud/vendor/jolicode/slack-php-api/generated/Normalizer/ObjsFileNormalizer.php on line 397

  at vendor/jolicode/slack-php-api/generated/Model/ObjsFile.php:1132
    1128|     {
    1129|         return $this->timestamp;
    1130|     }
    1131|
  > 1132|     public function setTimestamp(?int $timestamp): self
    1133|     {
    1134|         $this->timestamp = $timestamp;
    1135|
    1136|         return $this;

I've updated this PR following the new method you referenced

[xavierlacot]

Hi @devsibwarra

The patch file changes look weird - there shouldn't be so many changed lines. Are you sure that you re-generated the patch based on the sole modifications listed in the slack-openapi-patched.json file?

Also, there should be changes in the generated directory, I guess. Can you please make sure to update the dependencies (composer update), then regenerate the SDK (vendor/bin/jane-openapi generate -c .jane-openapi.php)?

Thanks again,

[devsibwarra]

The patch file changes look weird - there shouldn't be so many changed lines. Are you sure that you re-generated the patch based on the sole modifications listed in the slack-openapi-patched.json file?

Yeah, I thought so as well but figured the patch generator knew best. I caught the branch up to master, re-applied our changes to resources/slack-openapi-patched.json, then ran ./bin/slack-api-client-generator spec:generate-patch.

Also, there should be changes in the generated directory, I guess. Can you please make sure to update the dependencies (composer update), then regenerate the SDK (vendor/bin/jane-openapi generate -c .jane-openapi.php)?

Yep. I'll do that now. Wasn't sure you wanted a PR to include generated files

[devsibwarra]

Updated PR using these steps

git rebase -i upstream/master
  ^ dropped all changes
composer update
./bin/slack-api-client-generator spec:update
# update resources/slack-openapi-patched.json
vendor/bin/jane-openapi generate --config-file=.jane-openapi.php
./bin/slack-api-client-generator spec:generate-patch

The slack-openapi-sorted.patch still has a large change set that seems like the patch generator is just reorganizing sections. Happy to try other suggestions or you're welcome to take a crack at it

[xavierlacot]

@devsibwarra I bet that you did not correctly rebase.

here are the steps to follow:

1. start from the current master branch
2. apply the following changes in the resources/slack-openapi-patched.json file:

diff --git a/resources/slack-openapi-patched.json b/resources/slack-openapi-patched.json
index 57fb9dd..ba09546 100644
--- a/resources/slack-openapi-patched.json
+++ b/resources/slack-openapi-patched.json
@@ -399,7 +399,7 @@
                     "type": "array"
                 },
                 "timestamp": {
-                    "type": "integer"
+                    "type": "{}"
                 },
                 "user": {
                     "$ref": "#/definitions/defs_user_id"
@@ -1032,13 +1032,13 @@
                     "type": "string"
                 },
                 "timestamp": {
-                    "type": "integer"
+                    "type": "{}"
                 },
                 "title": {
                     "type": "string"
                 },
                 "updated": {
-                    "type": "integer"
+                    "type": "{}"
                 },
                 "url_private": {
                     "format": "uri",

3. re-generate the SDK:

   $ vendor/bin/jane-openapi generate --config-file=.jane-openapi.php
   $ git status
   modified :         generated/Model/ObjsComment.php
   modified :         generated/Model/ObjsFile.php
   modified :         resources/slack-openapi-patched.json

4. regenerate the patch file:

   $ ./bin/slack-api-client-generator spec:generate-patch
   $ git status
   modified :         generated/Model/ObjsComment.php
   modified :         generated/Model/ObjsFile.php
   modified :         resources/slack-openapi-patched.json
   modified :         resources/slack-openapi-sorted.patch

Here is how the resulting patch file should be modified:

git diff resources/slack-openapi-sorted.patch

gives:

diff --git a/resources/slack-openapi-sorted.patch b/resources/slack-openapi-sorted.patch
index a2a044c..9568bd3 100644
--- a/resources/slack-openapi-sorted.patch
+++ b/resources/slack-openapi-sorted.patch
@@ -1,5 +1,28 @@
---- resources/slack-openapi-sorted.json        2020-08-04 19:14:54.912874701 +0200
-+++ resources/slack-openapi-patched.json       2020-08-04 20:58:05.960245376 +0200
+--- resources/slack-openapi-sorted.json        2020-08-25 20:25:00.228618501 +0200
++++ resources/slack-openapi-patched.json       2020-08-27 14:51:04.219871193 +0200
+@@ -392,21 +392,21 @@
+                     },
+                     "type": "array"
+                 },
+                 "reactions": {
+                     "items": {
+                         "$ref": "#/definitions/objs_reaction"
+                     },
+                     "type": "array"
+                 },
+                 "timestamp": {
+-                    "type": "integer"
++                    "type": "{}"
+                 },
+                 "user": {
+                     "$ref": "#/definitions/defs_user_id"
+                 }
+             },
+             "required": [
+                 "comment",
+                 "created",
+                 "id",
+                 "is_intro",
 @@ -415,763 +415,331 @@
              ],
              "title": "File Comment Object",
@@ -1157,6 +1180,36 @@
                      "format": "uri",
                      "type": "string"
                  },
+@@ -1425,27 +1025,27 @@
+                 "thumb_960_h": {
+                     "type": "integer"
+                 },
+                 "thumb_960_w": {
+                     "type": "integer"
+                 },
+                 "thumb_tiny": {
+                     "type": "string"
+                 },
+                 "timestamp": {
+-                    "type": "integer"
++                    "type": "{}"
+                 },
+                 "title": {
+                     "type": "string"
+                 },
+                 "updated": {
+-                    "type": "integer"
++                    "type": "{}"
+                 },
+                 "url_private": {
+                     "format": "uri",
+                     "type": "string"
+                 },
+                 "url_private_download": {
+                     "format": "uri",
+                     "type": "string"
+                 },
+                 "user": {
 @@ -1674,44 +1274,117 @@
                  "user"
              ],

5. you're done, simply push only these 4 file changes.

[devsibwarra]

Hi @xavierlacot

Thanks for the update. I could've sworn I rebased right, but starting from scratch this time and the generated file list looks good now.

The resources/slack-openapi-sorted.patch file still has more modifications than your diff shows. Is there a specific PHP version / module I should be using to run ./bin/slack-api-client-generator spec:generate-patch?

[damienalexandre]

Hi everyone, just took the liberty to open a new PR with a fix for this issue: https://github.com/jolicode/slack-php-api/pull/102

Thanks a lot for bringing this issue up and for the time and effort put into fixing it :+1: Cheers!

[damienalexandre]

If you want you can try out my patch by installing the dev-master version of this library :tada: