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

Get values from getAttributes instead of only method #50

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

Kyzegs
Copy link
Contributor

@Kyzegs Kyzegs commented Aug 28, 2023

array_intersect_key($this->getAttributes(), array_flip($changedKeys) retrieves the values already casted:

array:13 [▼ // vendor\overtrue\laravel-versionable\src\Versionable.php:205
  "name" => "Anime Expo"
  "description" => "ASDsdfdfg"
  "size" => "SMALL"
  "location" => "8441 New Salem St, San Diego, CA 92126, USA"
  "country_code" => "AS"
  "continent_code" => "EU"
  "url" => "https://libertas.gg"
  "thumbnail_url" => "conventions/thumbnails/20HiotL5hhsNPD7DgbvcpmcelMNEuO9jHtGSqjYn.png"
  "banner_url" => "conventions/banners/vjZYaxELnNdzxsszGIZxOYwqaYx29Bcfr4ZTHAPp.png"
  "slug" => "anime-expo-4"
  "updated_at" => "2023-08-28 18:21:58"
  "created_at" => "2023-08-28 18:21:58"
  "id" => 317
]

Whereas $this->only($changedKeys) retrieves the non-casted values:

array:13 [▼ // vendor\overtrue\laravel-versionable\src\Versionable.php:205
  "name" => "Anime Expo"
  "description" => "ASDsdfdfg"
  "size" => App\Enums\ConventionSize {#1539 ▶
    +name: "SMALL"
    +value: "SMALL"
  }
  "location" => "8441 New Salem St, San Diego, CA 92126, USA"
  "country_code" => "AS"
  "continent_code" => "EU"
  "url" => "https://libertas.gg"
  "thumbnail_url" => Illuminate\Http\UploadedFile {#1535 ▶
    -test: false
    -originalName: "Daijin.jpg"
    -mimeType: "application/octet-stream"
    -error: 0
    #hashName: "20HiotL5hhsNPD7DgbvcpmcelMNEuO9jHtGSqjYn"
    path: "C:\Users\Sebastiaan\AppData\Local\Temp"
    filename: "phpBDF9.tmp"
    basename: "phpBDF9.tmp"
    pathname: "C:\Users\Sebastiaan\AppData\Local\Temp\phpBDF9.tmp"
    extension: "tmp"
    realPath: "C:\Users\Sebastiaan\AppData\Local\Temp\phpBDF9.tmp"
    aTime: 2023-08-28 18:21:58
    mTime: 2023-08-28 18:21:58
    cTime: 2023-08-28 18:21:58
    inode: 17169973579799816
    size: 367239
    perms: 0100666
    owner: 0
    group: 0
    type: "file"
    writable: true
    readable: true
    executable: false
    file: true
    dir: false
    link: false
    linkTarget: "C:\Users\Sebastiaan\AppData\Local\Temp\phpBDF9.tmp"
  }
  "banner_url" => Illuminate\Http\UploadedFile {#1536 ▶
    -test: false
    -originalName: "Lucy (2).png"
    -mimeType: "application/octet-stream"
    -error: 0
    #hashName: "vjZYaxELnNdzxsszGIZxOYwqaYx29Bcfr4ZTHAPp"
    path: "C:\Users\Sebastiaan\AppData\Local\Temp"
    filename: "phpBDFA.tmp"
    basename: "phpBDFA.tmp"
    pathname: "C:\Users\Sebastiaan\AppData\Local\Temp\phpBDFA.tmp"
    extension: "tmp"
    realPath: "C:\Users\Sebastiaan\AppData\Local\Temp\phpBDFA.tmp"
    aTime: 2023-08-28 18:21:58
    mTime: 2023-08-28 18:21:58
    cTime: 2023-08-28 18:21:58
    inode: 8725724279018095
    size: 2754423
    perms: 0100666
    owner: 0
    group: 0
    type: "file"
    writable: true
    readable: true
    executable: false
    file: true
    dir: false
    link: false
    linkTarget: "C:\Users\Sebastiaan\AppData\Local\Temp\phpBDFA.tmp"
  }
  "slug" => "anime-expo-4"
  "updated_at" => Illuminate\Support\Carbon @1693246918 {#1625 ▶
    #endOfTime: false
    #startOfTime: false
    #constructedObjectId: "00000000000006590000000000000000"
    #localMonthsOverflow: null
    #localYearsOverflow: null
    #localStrictModeEnabled: null
    #localHumanDiffOptions: null
    #localToStringFormat: null
    #localSerializer: null
    #localMacros: null
    #localGenericMacros: null
    #localFormatFunction: null
    #localTranslator: null
    #dumpProperties: array:3 [▶
      0 => "date"
      1 => "timezone_type"
      2 => "timezone"
    ]
    #dumpLocale: null
    #dumpDateProperties: null
    date: 2023-08-28 18:21:58.0 UTC (+00:00)
  }
  "created_at" => Illuminate\Support\Carbon @1693246918 {#1615 ▶
    #endOfTime: false
    #startOfTime: false
    #constructedObjectId: "000000000000064f0000000000000000"
    #localMonthsOverflow: null
    #localYearsOverflow: null
    #localStrictModeEnabled: null
    #localHumanDiffOptions: null
    #localToStringFormat: null
    #localSerializer: null
    #localMacros: null
    #localGenericMacros: null
    #localFormatFunction: null
    #localTranslator: null
    #dumpProperties: array:3 [▶
      0 => "date"
      1 => "timezone_type"
      2 => "timezone"
    ]
    #dumpLocale: null
    #dumpDateProperties: null
    date: 2023-08-28 18:21:58.0 UTC (+00:00)
  }
  "id" => 317
]

The currently implemented solutin will therefore sometimes result into invalid content:

{
    "id":301,
    "url":"https://libertas.gg",
    "name":"Anime Expo",
    "size":"SMALL",
    "slug":"anime-expo",
    "location":"1300 Pennsylvania Avenue NW, Washington, DC 20004, USA",
    // Empty object instead of string... 
    "banner_url":{
        
    },
    "created_at":"2023-08-28T09:37:36.000000Z",
    "updated_at":"2023-08-28T09:37:36.000000Z",
    "description":"Testing",
    "country_code":"AS",
    // Empty object instead of string... 
    "thumbnail_url":{
        
    },
    "continent_code":"EU"
}

@overtrue overtrue merged commit c91b5f4 into overtrue:4.x Aug 28, 2023
1 check passed
@Kyzegs Kyzegs deleted the casted-attributes branch August 29, 2023 06:51
@tasinttttttt
Copy link
Contributor

⚠️ This breaks behavior when dealing with cast arrays! ⚠️

An example is found in the Feature test post_can_revert_to_target_version

In the tests, there's a json field called extends which is cast as an array. With the change introduced here, the cast is not taken into account:

Failed asserting that '{"foo":"bar"}' is identical to Array &0 [
    'foo' => 'bar',
].

Makes me think of the merge process, I don't think tests are run before merging right?

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.

3 participants