laravel-schedule-monitor icon indicating copy to clipboard operation
laravel-schedule-monitor copied to clipboard

Store schedule monitoring configurations in its own singleton

Open m-bymike opened this issue 1 year ago • 1 comments

Instead of declaring runtime properties in the macros, we delegate storage of configuration properties to an extra class.

This aims at issue #103.

I open this pull request with a potential solution to #103, and I'm very open to discuss my approach. It should serve as a conversation starter, I don't consider it done by now :).

m-bymike avatar Aug 05 '24 09:08 m-bymike

:)

m-bymike avatar Sep 09 '24 09:09 m-bymike

Hi @freekmurze ,

thank you for the review, I addressed all suggested changes.

So far I tested the changes only locally (including the latest version) in a large project where it works as expected.

m-bymike avatar Dec 25 '24 13:12 m-bymike

Seems that the tests are failing, could you take a look at that too?

freekmurze avatar Dec 30 '24 11:12 freekmurze

Seems that the tests are failing, could you take a look at that too?

Just did, one of them uncovered a line that I missed, which has now been fixed (534e4e07eec4d8a1702f4406ef263ef4620e624a). The other failing test was because of a difference in the name of the testing environment. I chose to use a dynamic way to retrieve the name of the current environment (00045a40ed70a51001e938ea4592a22efdc8f242).

Then I figured that the tests only include PHP 8.0 - 8.2, so I added 8.3 and 8.4. At the same time I removed 8.0 as it is EoL (3811a44f90de4a176d02ee7e23896fed1903d028).

And I also updated the PHP version requirement in the composer.json file to ^8.1 (f0298c0324ae8f6e28c00b937306f8ef22d56b09).

m-bymike avatar Dec 30 '24 19:12 m-bymike

Thanks!

freekmurze avatar Jan 06 '25 15:01 freekmurze