Skip to content

Conversation

@iBotPeaches
Copy link
Member

  • Since we dropped less than 8 - refactoring to support some newer language features.
  • Also prevents hard-coding of User model via static property on Logger

@iBotPeaches iBotPeaches marked this pull request as ready for review January 15, 2023 14:10
* @return int|null
*/
public static function findUserId($model): ?int
public static function findUserId(Model|null $model): ?int
Copy link
Contributor

Choose a reason for hiding this comment

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

What about nullable type hint?

Suggested change
public static function findUserId(Model|null $model): ?int
public static function findUserId(?Model $model): ?int

//--------------------------------------------------------------------------------------------------------------

protected function setIpAddressAttribute($value)
protected function setIpAddressAttribute(string|null $value): void
Copy link
Contributor

Choose a reason for hiding this comment

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

Nullable type hint here too?

Suggested change
protected function setIpAddressAttribute(string|null $value): void
protected function setIpAddressAttribute(?string $value): void

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this just sugar? Or is anything different from the two? Just curious as ?|string is not a valid docblock, but string|null is.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's syntactic sugar

A single base type declaration can be marked nullable by prefixing the type with a question mark (?). Thus ?T and T|null are identical.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so it seems it works in userland, just docblock parsing doesn't work yet for those - https://github.com/laravel/framework/pull/45677/files

ActivityType::PASSWORD_CHANGE => trans('logger::enums.activity_type_password_change'),
ActivityType::GET_DATA => trans('logger::enums.activity_type_get_data'),
ActivityType::MODIFY_DATA => trans('logger::enums.activity_type_modify_data'),
default => throw new \Exception('Unknown enum type: ' . $this->type),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we simplify the namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll bring up a new PR to handle this and the nullable typehints

@iBotPeaches iBotPeaches merged commit adf187a into master Jan 17, 2023
@iBotPeaches iBotPeaches deleted the php8-alignment branch January 17, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants