Added Timebomb functionality to Features

[AidanLaycock]

This PR added the ability to set a feature to expire during creation. It then throws an exception, designed to break CI so that technical debt can be removed.

Users can also create long living Features when using Timebombs by leaving an expiry as null (This could be useful for Beta Features).

[ksassnowski]

I feel like this is rather dangerous in its current state as it could make the production system randomly explode unless you remembered to deploy a new version in time. Ideally, this should only happen during CI (so probably depending on APP_ENV). What do you think?

[ksassnowski]

This might come down to a stylistic choice but I'm not a big fan of the dummy $this->assertTrue(true) assertions to assert that no exception was thrown. The reason is that you're not really asserting that no exception was thrown, you're asserting that true === true and sort of implicitly rely on the fact that in order to even reach this code, no exception could have been thrown.

My suggestion would be to replace all the $this->assertTrue(true) calls with the following:

it('should not throw an exception', function () {
    $exception = null;

    try {
        $foo->methodThatMightThrowException();
    catch (Throwable $exception) {
        // If an exception was thrown, it will be assigned to `$exception`.
        //
        // To make it more explicit what's happenning, you could alternatively 
        // use two different variables for the exception and explicitly assign 
        // the caught exception to the outer variable.
    }

    // Explicitly test that no exception was caught.
    assertNull($e, 'Unexpected assertion was thrown');
});

While this is rather verbose, it very explicitly asserts against the thing the test is claiming to test: that there was no exception.

Some food for thought :)

[AidanLaycock]

Hey @ksassnowski - Thanks for the feedback, I'll have a look at those later today and update the PR :)

[ksassnowski]

Just FYI, I'm not a maintainer of this project so these are just suggestions. @JustSteveKing has the final say :)

[JustSteveKing]

I think @ksassnowski has brought up some good points worth looking at. The last thing I want to do is to introduce changes where production systems suddenly start exploding because of the time bomb feature.

Good catch on the tests too!

[AidanLaycock]

@ksassnowski - Thank you again for the feedback :) I've implemented those points.

@JustSteveKing - This should hopefully be ready to be checked/merged now, let me know if you want anything else adding, changing! :)

[ksassnowski]

I find the name time_bomb_environments a little counter-intuitive. Without reading the comment, I would have assumed that these are the environments in which the time bombs are active, not the other way around.

Another thing is that my first instinct have would be to have the time bombs be deactivated in every environment and to explicitly activate them for certain environments. I think this would reduce the chance of accidentally activating it in production because of a typo or something similar.

[JustSteveKing]

I have to agree here, time bomb feature should be enabled per environment and off by default - not something you would typically have in staging/qa

[AidanLaycock]

@ksassnowski @JustSteveKing - Thank you for the feedback. Do you think if the 'enable_time_bombs' had an environment variable this would resolve this? As that value governs whether the feature is on/off at all.

We're using trunk based development which is why I considered having an exclude rather than include list, as we'd want it to be on all of the time except for production rather than having to specify on for dev, testing, qa, staging.

[AidanLaycock]

Hey @JustSteveKing - I was wondering if you'd had a chance to have another look at this, and whether it could be merged?

[AidanLaycock]

Hi @JustSteveKing - Just bumping this again to check whether you've had a chance to have a look over it? Or would it be worth me forking this and maintaining a version for ourselves?

[JustSteveKing]

Hey @AidanLaycock

Sorry my GitHub notifications seem to be having a bit of a funny time!

So ideally I would like to see the time bomb feature as opt in, based on an ENV variable, with a default set to disable them. I love this idea, but introducing it when it's already published means it needs to mirror what's there without breaking where possible

[AidanLaycock]

Hi @JustSteveKing - It's quite alright! :)

And that's understandable! It's currently setup that way so that it's off by default - https://github.com/JustSteveKing/laravel-feature-flags/pull/21/files#diff-533ac2bfa60c5ef6fed2da48ef0c90f7494b91dd0340eba01de9ec1c224e6785R50

If you're happy with the rest of the implementation then I'll have a look over the docs so that they are up to date with the new functionality then I can push that tomorrow morning for you.

[JustSteveKing]

Go for it @AidanLaycock - I will keep my eye on PRs tomorrow 💪

[AidanLaycock]

Hey @JustSteveKing - That's great, this should hopefully be ready for merging now! I've added more info in the ReadMe about Timebombs. And I've also updated a few return types that I spotted whilst reopening the repo.

Do let me know if you want anything else changing! And please feel free to rework the readMe as you see fit!

[JustSteveKing]

Merged and released! Thanks @AidanLaycock