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

Simplify implementation #56

Merged
merged 23 commits into from
Aug 24, 2021

Conversation

chapterjason
Copy link
Contributor

@chapterjason chapterjason commented Aug 21, 2021

List of the most important changes (From the commits but without Fix CS stuff commits.):

  • refactor!: Rename addCompilerPasses to addCompilerPass
    BREAKING CHANGE: Renamed addCompilerPasses to addCompilerPass, this also changed the signature to single compiler passes only.
  • refactor!: Rename setRoutingFile to addRoutingFile
    BREAKING CHANGE: Renamed setRoutingFile to addRoutingFile to allow multiple routing files
  • refactor!: Rename addConfigFile to addConfig
    BREAKING CHANGE: Renamed addConfigFile to addConfig cause it also takes a closure to allow configuration with the Symfony\Component\DependencyInjection\ContainerBuilder
  • refactor!: Rename AppKernel to TestKernel
    BREAKING CHANGE: Renamed AppKernel to TestKernel to make it more obvious what the kernel is used for.
  • refactor: Switch to configure kernel class via method instead override static property
  • refactor: Use MicroKernelTrait instead of custom implementation
  • refactor: Use yield instead of temporary array
  • fix: Add realpath for cache and log dir
    This resolves symlinks that might be used in systems like MacOS
  • cut: Remove setRootDir method
    The rootDir is already deprecated lower than symfony 4.4, no need to support.
  • ci: Update to latest ubuntu
  • chore: Add types on several places
  • chore: Update php-cs-fixer

Propery and method names

I also thought about changing the properties something like:

    private array $testBundles = [];
    private array $testConfigs = [];
    private array $testRoutingFiles = [];
    private array $testCompilerPasses = [];
    private string $testCachePrefix;
    private ?string $testProjectDirectory = null;

This will ensure they will, should, never conflict, and it helps while maintaining to differ between Symfony properties and own properties.

Also for the related methods I would prefix them like addTestRoutingFile, also this in the public api makes it more obvious for the developers what they are working with.

WDYT?

Readme

Using this bundle test together with Matthias Nobacks's
SymfonyDependencyInjectionTest
will give you a good base for testing a Symfony bundle.

Theres still that mention in the readme, I don't think that this will still be compatible.
I think we could also refactor a lot there to traits like ContainerAssertionsTrait

Also for the ConfigTest package.

Related to #55

BREAKING CHANGE: Renamed AppKernel to TestKernel to make it more obvious what the kernel is used for.
BREAKING CHANGE: Renamed addConfigFile to addConfig cause it also takes a closure to allow configuration with the Symfony\Component\DependencyInjection\ContainerBuilder
BREAKING CHANGE: Renamed setRoutingFile to addRoutingFile to allow multiple routing files
The rootDir is already deprecated lower than symfony 4.4, no need to support.
This resolves symlinks that might be used in systems like MacOS
BREAKING CHANGE: Renamed addCompilerPasses to addCompilerPass, this also changed the signature to single compiler passes only.
@chapterjason chapterjason requested a review from Nyholm August 21, 2021 01:56
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work.

I like this.
Im happy to merge this now and then update readme and properties/methods in another PR.

.github/workflows/static.yml Outdated Show resolved Hide resolved
@chapterjason chapterjason requested a review from Nyholm August 24, 2021 15:19
@Nyholm Nyholm merged commit 5ce337b into SymfonyTest:master Aug 24, 2021
@Nyholm
Copy link
Member

Nyholm commented Aug 24, 2021

Thank you

@chapterjason chapterjason deleted the feature/simplification branch August 24, 2021 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants