sandreas / m4b-tool

m4b-tool is a command line utility to merge, split and chapterize audiobook files such as mp3, ogg, flac, m4a or m4b
MIT License
1.18k stars 76 forks source link

Deprecation warnings running php v8? #258

Closed moviebrain closed 6 months ago

moviebrain commented 6 months ago

Would there be any pushback if I attempted a PR to resolve some of the deprecation warnings I see running on php v8? I don't know how quickly php dev moves, but it is my understanding a good number of these will become hard breaks in php v9?

For the latter trim() warning (the very last appears to be the code checking if a mp3 tag exists?) , it looks like a null coalescing assignment operator will work for > php7, but I don't want to try and make a PR that is breaking for anyone, or if it's not really wanted by anyone. I am not really certain what the targeted runtime is, and I don't want to try this and break stuff without knowing any better.

Maybe it'd just be QoL and I'm the only one who gets that squiggle feeling when the terminal pukes out pages of warnings?

PHP Deprecated: Return type of Sandreas\Time\TimeUnit::jsonSerialize() should either be compatible with JsonSerializable::jsonSerialize(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///opt/homebrew/bin/m4b-tool/vendor/sandreas/php-time/src/Sandreas/Time/TimeUnit.php on line 241

PHP Deprecated: Return type of M4bTool\Audio\Tag::offsetExists($offset) should either be compatible with ArrayAccess::offsetExists(mixed $offset): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///opt/homebrew/bin/m4b-tool/src/library/Audio/Tag.php on line 225

PHP Deprecated: Return type of M4bTool\Audio\Tag::offsetGet($offset) should either be compatible with ArrayAccess::offsetGet(mixed $offset): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///opt/homebrew/bin/m4b-tool/src/library/Audio/Tag.php on line 239

PHP Deprecated: Return type of M4bTool\Audio\Tag::offsetSet($offset, $value) should either be compatible with ArrayAccess::offsetSet(mixed $offset, mixed $value): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///opt/homebrew/bin/m4b-tool/src/library/Audio/Tag.php on line 256

PHP Deprecated: Return type of M4bTool\Audio\Tag::jsonSerialize() should either be compatible with JsonSerializable::jsonSerialize(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///opt/homebrew/bin/m4b-tool/src/library/Audio/Tag.php on line 284

PHP Deprecated: trim(): Passing null to parameter #1 ($string) of type string is deprecated in phar:///opt/homebrew/bin/m4b-tool/src/library/Command/AbstractCommand.php on line 482

PHP Deprecated: Implicit conversion from float 5542314.149999999 to int loses precision in phar:///opt/homebrew/bin/m4b-tool/src/library/Parser/SilenceParser.php on line 71

PHP Deprecated: trim(): Passing null to parameter #1 ($string) of type string is deprecated in phar:///opt/homebrew/bin/m4b-tool/src/library/Command/AbstractCommand.php on line 482

PHP Deprecated: trim(): Passing null to parameter #1 ($string) of type string is deprecated in phar:///opt/homebrew/bin/m4b-tool/src/library/Executables/Mp4tags.php on line 95

JakobTischler commented 6 months ago

I for one would really appreciate fixes for these. I use merge in a chain of commands, which currently can't proceed after the merge because of the error result.

sandreas commented 6 months ago

Would there be any pushback if I attempted a PR to resolve some of the deprecation warnings I see running on php v8?

No actually this would be great. One minor detail: I'm into this right now (currently analyzing) and I would like to prevent you working on this just to see in a week that there have been updates. I would love to invest more time on this, but since it is started, I will perform my commits. After this you are welcome to do a PR.

I'm totally aware that m4b-tool needs some love again and since there is so much deprecated stuff, this is something that needs to be done as soon as possible.

As a workaround you could either change your php.ini:

error_reporting=E_ALL & ~E_DEPRECATED

or run php with inline ini options:

php -d "error_reporting=E_ALL & ~E_DEPRECATED" m4b-tool.phar #...

I would love to get some feedback on this.

moviebrain commented 6 months ago

Would there be any pushback if I attempted a PR to resolve some of the deprecation warnings I see running on php v8?

No actually this would be great. One minor detail: I'm into this right now (currently analyzing) and I would like to prevent you working on this just to see in a week that there have been updates. I would love to invest more time on this, but since it is started, I will perform my commits. After this you are welcome to do a PR.

I'm totally aware that m4b-tool needs some love again and since there is so much deprecated stuff, this is something that needs to be done as soon as possible.

As a workaround you could either change your php.ini:

error_reporting=E_ALL & ~E_DEPRECATED

or run php with inline ini options:

php -d "error_reporting=E_ALL & ~E_DEPRECATED" m4b-tool.phar #...

I would love to get some feedback on this.

Excellent. I’ll circle back around to this next weekend when I have some time then, see where it’s at. Thanks for the reply.

sandreas commented 6 months ago

@JakobTischler @moviebrain @tommorris The marked commit above fixes some of the deprecations, but also alters the error_reporting to no longer contain E_DEPRECATED via:


// disable E_DEPRECATED errors
if(!getenv("M4B_TOOL_FORCE_INI_ERROR_REPORTING")) {
    $errorReportingLevel = error_reporting();
    error_reporting($errorReportingLevel & ~E_DEPRECATED);
}

This basically means it takes the current error_reporting level of the php.ini and removes the E_DEPRECATED flag, as long as you not set the environment variable M4B_TOOL_FORCE_INI_ERROR_REPORTING=1. I hope this will meet your requirements to not have this absolutely useless error messages in your command line and has no side effects (it did not have in my tests).

One deprecation message is caused by a dependency TimeUnit (php-time package), that is also my work. I'll try to update the package asap.

In the next weeks I'll try to make m4b-tool as php8 ready as possible without breaking stuff. If you can't build m4b-tool yourselves, I try to publish a new pre-release in the next days containing these code updates.

JakobTischler commented 6 months ago

Thank you very much for the help and your work in general.

moviebrain commented 6 months ago

Thanks @sandreas for all this work. It is greatly appreciated!

sandreas commented 6 months ago

@moviebrain @JakobTischler @tommorris New Pre-Release is out - let me know if it works.

abhilesh commented 6 months ago

Happy to report that deprecation warnings are gone in the latest Pre-Release version! Thanks a lot!

sandreas commented 6 months ago

Happy to report that deprecation warnings are gone in the latest Pre-Release version! Thanks a lot!

Great :-) Thanks for the feedback.

csandman commented 1 month ago

I have a somewhat related question about this, is it possible to use the alpine package php81 in place of php8? I have a docker project that depends on this project, and I really need to upgrade the version of alpine that my project uses, but the latest version that still includes php8 is v3.16. So I was just curious if 8.1 is a drop in replacement or not before I go down the rabbithold of trying it.

sandreas commented 1 month ago

@csandman Thanks for pointing out. I think it is time for an upgrade. php 8.1 should work exactly the same. Maybe I'm going to change the base image like this:

I did not test it, but the future Dockerfile will be probably look somewhat to this:

FROM sandreas/ffmpeg:5.0.1-3 as ffmpeg
FROM sandreas/tone:v0.1.7 as tone
FROM sandreas/mp4v2:2.1.1 as mp4v2
FROM sandreas/fdkaac:2.0.1 as fdkaac
FROM php:cli-alpine
ENV WORKDIR /mnt/
ENV M4BTOOL_TMP_DIR /tmp/m4b-tool/

RUN echo "---- INSTALL RUNTIME PACKAGES ----" && \
  apk add --no-cache --update --upgrade \
  # mp4v2: required libraries
  libstdc++

COPY --from=ffmpeg /usr/local/bin/ffmpeg /usr/local/bin/
COPY --from=tone /usr/local/bin/tone /usr/local/bin/
COPY --from=mp4v2 /usr/local/bin/mp4* /usr/local/bin/
COPY --from=mp4v2 /usr/local/lib/libmp4v2* /usr/local/lib/
COPY --from=fdkaac /usr/local/bin/fdkaac /usr/local/bin/

ARG M4B_TOOL_DOWNLOAD_LINK="https://github.com/sandreas/m4b-tool/releases/latest/download/m4b-tool.tar.gz"

RUN echo "---- INSTALL M4B-TOOL ----" \
    && if [ ! -f /tmp/m4b-tool.phar ]; then \
            echo "!!! DOWNLOADING ${M4B_TOOL_DOWNLOAD_LINK} !!!" && wget "${M4B_TOOL_DOWNLOAD_LINK}" -O /tmp/m4b-tool.tar.gz && \
            if [ ! -f /tmp/m4b-tool.phar ]; then \
                tar xzf /tmp/m4b-tool.tar.gz -C /tmp/ && rm /tmp/m4b-tool.tar.gz ;\
            fi \
       fi \
    && mv /tmp/m4b-tool.phar /usr/local/bin/m4b-tool \
    && M4B_TOOL_PRE_RELEASE_LINK=$(wget -q -O - https://github.com/sandreas/m4b-tool/releases/tag/latest | grep -o 'M4B_TOOL_DOWNLOAD_LINK=[^ ]*' | head -1 | cut -d '=' -f 2) \
    && echo "!!! DOWNLOADING PRE_RELEASE ${M4B_TOOL_DOWNLOAD_LINK} !!!" && wget "${M4B_TOOL_PRE_RELEASE_LINK}" -O /tmp/m4b-tool.tar.gz \
    && tar xzf /tmp/m4b-tool.tar.gz -C /tmp/ && rm /tmp/m4b-tool.tar.gz \
    && mv /tmp/m4b-tool.phar /usr/local/bin/m4b-tool-pre \
    && chmod +x /usr/local/bin/m4b-tool /usr/local/bin/m4b-tool-pre

WORKDIR ${WORKDIR}
CMD ["list"]
ENTRYPOINT ["m4b-tool-pre"]