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

Make it possible to extend SetupScript #3643

Merged
merged 3 commits into from
Sep 23, 2022
Merged

Make it possible to extend SetupScript #3643

merged 3 commits into from
Sep 23, 2022

Conversation

iPurpl3x
Copy link
Contributor

@iPurpl3x iPurpl3x commented Sep 16, 2022

Changes proposed in this pull request:

Make it possible to extend Flarum\Testing\integration\Setup\SetupScript and added a public method to add settings to the initial installation pipeline.

New usage example:

(new SetupScript())
    ->addSettings([...])
    ->run();

As documented, the new method can be used to add settings to the Flarum installation.
Should be used only when it is really needed.
This can be useful in rare cases where the settings are required to be set already when extensions Extenders are executed. In those cases, setting the settings with the setting() method of the TestCase will not work.

Reviewers should focus on:
The new methods work and don't break any existing code.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

…pt and added public methods to add settings or extensions to in initial installation pipeline
@iPurpl3x iPurpl3x requested a review from a team as a code owner September 16, 2022 12:26
@SychO9
Copy link
Member

SychO9 commented Sep 16, 2022

What's the use case for this? I'd rather make sure the current codebase doesn't allow you to achieve your use case before making the change.

@iPurpl3x
Copy link
Contributor Author

Hi @SychO9

In my use-case, I needed to activate some extensions (like flarum/tags) because else, no matter if I did $this->extension('flarum-tags'); in the TestCase or not, the test would fail because a migration of the extension I am writing the test in, required flarum/tags to be enabled, as it alters the tag_user table. Seems like migrations are executed before $this->extension(...);

Also, I realized that making settings with $this->setting(...) was not helping to use glowingblue/redis-setup. But when I set ['glowingblue-redis.enableCache' => true] in the setup, it works as expected.

This is now my setup.php code:

$setup = (new SetupScript())
	->addSettings([
		'glowingblue-redis.enableCache' => true,
	])
	->addExtensions([
		'flarum-tags',
		'flarum-likes',
		'glowingblue-redis-setup',
	])
	->run();

@SychO9
Copy link
Member

SychO9 commented Sep 16, 2022

The extension and setting methods will have no effect if you are calling them after the application has already been booted, which is very likely what's happening in your tests, because otherwise, migrations run just fine for specified extensions, and some for settings.

Lookout for anything in your tests that could be causing your tests to boot the application earlier than you want, like resolving a class, or making manual database changes or an early request ..etc

@iPurpl3x
Copy link
Contributor Author

Yes I get that, so I need to change the code here : https://github.com/glowingblue/flarum-ext-redis-setup/blob/master/src/Extend/EnableRedis.php#L40 ?

And what about the migrations that fail ?

@SychO9
Copy link
Member

SychO9 commented Sep 16, 2022

I don't know how that ties into the test code so I can't tell, maybe if you share the test code in the discord server

@iPurpl3x
Copy link
Contributor Author

I made the update as discussed over Discord @SychO9. If you want to make any adjustments to the docblock I wrote, let me know.

@luceos luceos merged commit 7ce9d63 into flarum:main Sep 23, 2022
@luceos luceos mentioned this pull request Nov 10, 2022
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.

3 participants