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

Load export type ids dynamically #703

Merged
merged 5 commits into from Apr 25, 2019

Conversation

simone-gasparini
Copy link
Contributor

@simone-gasparini simone-gasparini commented Apr 13, 2019

Description

Load export type ids dynamically

Fixes #702

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have executed bin/console kimai:phpcs --fix to make sure my changes adopt the correct code style
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I agree that this code is used in Kimai and will be published under the MIT license

@kevinpapst
Copy link
Member

Fixing a test by deleting it is the worst possible solution. Can you please try to find tests for the new logic and check if you can fix the old one.

@simone-gasparini
Copy link
Contributor Author

I've restored the old test and added a foreach to add types. For the new implementation i think that the existing test it's already ok.
https://github.com/kevinpapst/kimai2/blob/master/tests/Controller/ExportControllerTest.php#L59

/**
* @return ExportQuery
*/
protected function createExportQueryWithExports()
Copy link
Member

Choose a reason for hiding this comment

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

I am not completely happy with this method being in the controller.
Maybe it could be moved as factory function to the ServiceExport. Registering the Query as a service could work as well and then registering the renderer type in a compiler pass.

I'll give it another thought and maybe push a change later on.

@kevinpapst kevinpapst added this to the 0.9 milestone Apr 25, 2019
@kevinpapst
Copy link
Member

I decided to remove the type validation from the query object, as it doesn't belong there.
And there was already another check in place in the controller... will merge now.

@kevinpapst kevinpapst merged commit f0f757c into kimai:master Apr 25, 2019
@simone-gasparini simone-gasparini deleted the feature/702_export_enhancement branch April 26, 2019 15:34
@lock
Copy link

lock bot commented Jun 25, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. If you use Kimai on a daily basis, please consider donating to support further development of Kimai.

@lock lock bot locked and limited conversation to collaborators Jun 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

Review export lifecycle for plugin injection
2 participants