Skip to content

Conversation

@perappu
Copy link

@perappu perappu commented Nov 29, 2025

  • Certain very popular extensions and other custom coded features have asset types that do not actually exist as a model (things such as EXP or points), and therefore do not have a rewardable_id. This migration changes the rewardable_id column to be nullable. Otherwise, it will cause the rewards to silently fail to save, because it can't create a null value in that column. I figured it was better to handle this in core than rely on various slightly different extensions or custom code solutions.
  • The item_id column does not exist and throws a SQL error if you tried to refer to it, so I changed it to object_id instead (this should serve the same purpose of creating a "null" relationship).

@itinerare itinerare added bug Something isn't working needs review Pull requests that are pending community review labels Nov 29, 2025
@perappu perappu changed the title Dynamic rewards fixes fix: dynamic rewards rewardable_id and item_id error Nov 29, 2025
@SpeedyD
Copy link
Contributor

SpeedyD commented Dec 1, 2025

Honestly, approved if only for the second edit- I think that's a bug to begin with, as 'item_id' is referenced NOWHERE else but ni that line.

Copy link
Contributor

@ScuffedNewt ScuffedNewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer this have some level of validation against null object_id for things like Items etc, it's unlikely to happen but I've seen people manage to do worse

Something as simple as a helper that returns if the object_id is allowed to be null or not on the reward model or the AssetsHelper

@SpeedyD
Copy link
Contributor

SpeedyD commented Dec 1, 2025

Would prefer this have some level of validation against null object_id for things like Items etc, it's unlikely to happen but I've seen people manage to do worse

Something as simple as a helper that returns if the object_id is allowed to be null or not on the reward model or the AssetsHelper

I am of strong believe that in Lorekeeper development, it is never a bad idea to PR a fix or give code suggestion :)

If you know how to fix this in a more efficient way, I doubt @perappu would mind you PRing it for their perappu:pr/dynamic-rewards-fixes branch :)

@perappu
Copy link
Author

perappu commented Dec 2, 2025

Would prefer this have some level of validation against null object_id for things like Items etc, it's unlikely to happen but I've seen people manage to do worse

Something as simple as a helper that returns if the object_id is allowed to be null or not on the reward model or the AssetsHelper

I don't think object_id is nullable? So it shouldn't be at all possible for it to turn out null. It'll be "0" at worst, which to my knowledge, when it comes to SQL comparisons is a non-null value (vs PHP ones). But we could certainly add validation rules on Reward which would prevent that from happening at all!

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

Labels

bug Something isn't working needs review Pull requests that are pending community review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants