JoliNotif icon indicating copy to clipboard operation
JoliNotif copied to clipboard

New notifer for Linux which use libnotify and PHP-FFI

Open morawskim opened this issue 1 year ago • 4 comments

Hello

This PR add another notifier, which use FFI and libnotify. Probably most of Linux notifier supported in this repository under the hood use this library.

This notifier has some advantages and disadvantages. Support for FFI in PHP vary between distributions. Sometimes we need to install dedicated package. In other hand can be slightly faster (we don't need to crate external process) and more portable (we don't need external application, only libnotify).

FFI allows us to call C code from PHP script. The FFI extension need to be installed to use this notifier. Also support for FFI is only in CLI SAPI, due to security concerns.

At the moment I tested this notifier with:

  • Ubuntu 20
  • Ubuntu 22
  • Ubuntu 24
  • Fedora 39
  • OpenSUSE Tumbleweed

All existing notifiers use external commands and maybe this new notifier does not meet project goals. I decided to mark this PR as draft. I can finish this PR if that notifier can be included in this package, otherwise you can close this PR.

morawskim avatar Mar 17 '24 09:03 morawskim

Hi @morawskim.

Thanks for your PR, it looks realy good. I will need to play a bit a with it but this is definitely something that could bin included in JoliNotif

pyrech avatar Mar 17 '24 11:03 pyrech

Hi @morawskim Do you have some time to finish this PR? It would be great to support a FFI-based notifier for linux environments.

pyrech avatar Apr 24 '24 20:04 pyrech

Yes, I will finish this PR. I should do this at the weekend.

morawskim avatar Apr 25 '24 18:04 morawskim

Hello @pyrech

I wanted to write some additional unit tests, but FFI class is final, so we cannot create mock class. I could create some FFI Wrapper and inject this in constructor for tests, but IMHO this would not give any value. If you have some ideas, I am very open for discussion.

I checked some potential memory leaks lastly. Valgrind report some memory leak, but not in libnotify. Also after running 400 notifications I didn't see any issues with memory.

morawskim avatar Apr 27 '24 18:04 morawskim

Thanks for your work so far. About the tests, I understand it's not ideal to unit test the FFI part. Instead maybe we could just trigger the notifier for real and ensure there is no exception (and skip the tests when the notifier is not available). It would be better than nothing. What do you think about it?

pyrech avatar Apr 28 '24 16:04 pyrech

Thanks for suggestion @pyrech

Instead maybe we could just trigger the notifier for real and ensure there is no exception (and skip the tests when the notifier is not available).

I have considered this, but I was afraid that this will require a lots of dependencies and the image will be huge.

But now.... I dig deeper and we can install only libnotify4 package in Ubuntu without any recommends and suggestions.

Simple test: Run container - docker run --rm -it -e PHP_EXTENSIONS=ffi -v $PWD:/usr/src/app thecodingmachine/php:8.3-v4-cli bash In container install only libnotify - sudo apt-get update -y && sudo apt-get install -y --no-install-recommends --no-install-suggests libnotify4 And in output you will see:

After this operation, 3470 kB of additional disk space will be used.

Amazing, we don't need install desktop environment, only one library. I added test for initialize the FFI. I have also updated GitHub workflow.

Runt tests ./vendor/bin/simple-phpunit tests/Notifier/LibNotifyNotifierTest.php

morawskim avatar Apr 28 '24 18:04 morawskim

I just rebased your PR in order to have PHPStan running in the CI. I also:

  • added a note in the Changelog
  • added a test testSendNotificationWithAllOptions that really triggers FFI and libnotify in the CI (even if the notifier returns false but I think that's expected since no desktop is installed on the CI, so I skip the test in this case but it works on my computer).

It worked out of the box on my computer, so congrats for your PR.

pyrech avatar Apr 29 '24 12:04 pyrech

Thanks a lot for your contribution @morawskim :pray:

pyrech avatar Apr 29 '24 18:04 pyrech