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

Remove \intval around MAX_IMAGE_BYTES in src/Service/SettingsManager.php, breaks settings name resolution #1011

Merged
1 commit merged into from
Aug 9, 2024

Conversation

ghost
Copy link

@ghost ghost commented Aug 9, 2024

Image uploads are currently broken, introduced by #997. PR removes it, settings can still be saved in settings page of admin panel.

image

Mentioned in #780 (comment) and confirmed in dev.

@ghost ghost added bug Something isn't working high priority critical issue or PR that impacts production labels Aug 9, 2024
@ghost ghost requested a review from BentiGorlich August 9, 2024 19:06
@ghost ghost self-assigned this Aug 9, 2024
@ghost ghost changed the title Remove \intval from MAX_IMAGE_BYTES, breaks name resolution Remove \intval around MAX_IMAGE_BYTES in src/Service/SettingsManager.php, breaks settings name resolution Aug 9, 2024
@ghost ghost enabled auto-merge (squash) August 9, 2024 19:14
@BentiGorlich
Copy link
Member

Why is intval breaking it though?

@BentiGorlich
Copy link
Member

I guess intval never returns null and therefore the default is not kicking in?

@ghost ghost merged commit aa954bc into main Aug 9, 2024
7 checks passed
@ghost ghost deleted the remove_intval_from_settings_manager_max_image_bytes branch August 9, 2024 19:16
@ghost
Copy link
Author

ghost commented Aug 9, 2024

I guess intval never returns null and therefore the default is not kicking in?

must be, Xdebug was showing 0 value assignment...

@BentiGorlich
Copy link
Member

I mean it is obviously not necessary if you don't get a type error...

@ghost ghost mentioned this pull request Aug 11, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority critical issue or PR that impacts production
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant