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

PHP 8: Fix major compatibility issues #216

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Conversation

jbeyerstedt
Copy link
Member

PHP8 deprecated, that class member variables can be created outside the class definition or constructor, which prevented the code to run at all. Additionally the error handling has changed, which has lead to multiple other errors during the runtime.

Finally, strftime was deprecated in PHP 8.1.

@jbeyerstedt
Copy link
Member Author

Don't know, if this breaks anything in php 7.4 (or earlier) as I don't have old php on my laptop any more.

@jbeyerstedt
Copy link
Member Author

Works with PHP8 on my machine and seems to work with the PHP7 docker container mentioned in the readme.

But there seems an issue to load player.js as it has a query param specifying the version number. But this also happens with the current master branch (with PHP in docker).

@jbeyerstedt
Copy link
Member Author

But there seems an issue to load player.js as it has a query param specifying the version number. But this also happens with the current master branch (with PHP in docker).

This should not be an issue as the assets are usually served by nginx directly. At least that the way how the LBs are doing it.

} else {
$rooms = isset($c->rooms->nodes) ? $c->rooms->nodes : [];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

It was so much shorter before 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

But I haven't found a shorter version which works and is readable.

Copy link
Member

@saerdnaer saerdnaer left a comment

Choose a reason for hiding this comment

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

LGTM

PHP8 deprecated, that class member variables can be created outside the class
definition or constructor, which prevented the code to run at all. Additionally
the error handling has changed, which has lead to multiple other errors during
the runtime.
Finally, strftime was deprecated in PHP 8.1.
The error control operator "@" has changed behavior in PHP8 and generally seems
like a quite bad hack. So let's handle array/ dict access errors explicitly.
@jbeyerstedt jbeyerstedt merged commit dc57652 into voc:master Oct 27, 2023
1 check failed
@jbeyerstedt jbeyerstedt deleted the fix/php8 branch October 27, 2023 18:20
DO1JLR added a commit to DO1JLR/voc-streaming-website-fork that referenced this pull request Oct 30, 2023
Beim testen des voc#223 ist mir aufgefallen, das zumindest für Debian bookworm die aktuelle php Version 8.2 ist.
Und seit voc#216 gemerged wurde, scheint php8 der aktuelle stand zu sein

Auch ist mir aufgefallen, das nicht nur ``php-curl`` und ``php-xml`` für das lokale testen benötigt werden, sondern auch das php paket. Ich habe es also in der README ergänzt.

Eventuell findet ihr diese PR nützlich und wollt ihn mergen?
saerdnaer pushed a commit that referenced this pull request Jan 14, 2024
Beim testen des #223 ist mir aufgefallen, das zumindest für Debian bookworm die aktuelle php Version 8.2 ist.
Und seit #216 gemerged wurde, scheint php8 der aktuelle stand zu sein

Auch ist mir aufgefallen, das nicht nur ``php-curl`` und ``php-xml`` für das lokale testen benötigt werden, sondern auch das php paket. Ich habe es also in der README ergänzt.

Eventuell findet ihr diese PR nützlich und wollt ihn mergen?
saerdnaer pushed a commit that referenced this pull request Jan 14, 2024
Beim testen des #223 ist mir aufgefallen, das zumindest für Debian bookworm die aktuelle php Version 8.2 ist.
Und seit #216 gemerged wurde, scheint php8 der aktuelle stand zu sein

Auch ist mir aufgefallen, das nicht nur ``php-curl`` und ``php-xml`` für das lokale testen benötigt werden, sondern auch das php paket. Ich habe es also in der README ergänzt.

Eventuell findet ihr diese PR nützlich und wollt ihn mergen?
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.

2 participants