-
Notifications
You must be signed in to change notification settings - Fork 35
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
Tried to make the bundle SF4 compatible #147
base: 4.0
Are you sure you want to change the base?
Conversation
The tests are still failing because of faulty config
Thanks for this work, ill try to check this on weekend. |
I'm currently using this dev version on a personal project of mine and it seems to work as expected (at least for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@helios-ag I made a quick review about only important things.
{ | ||
$result = new Decoda(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason to delete this case?
The assertion here is that no exception is thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not testing anything, since the constructor will never throw an error. In fact, why is third party code being tested? Also, PHPUnit sees it as a risky test, since nothing happens inside the test.
|
||
$result = new EmoticonHook($loader, $container); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason to delete this case?
The assertion here is that no exception is thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idem
public: true | ||
tags: | ||
- { name: fm_bbcode.decoda.filter, id: table } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicate service definition on Resources/config/filters.xml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I think I missed something somewhere. Pretty sure it was required for some reason, will look into it.
@@ -68,7 +66,7 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
} | |||
|
|||
if (!file_exists($emoticonsFolder) && !is_dir($emoticonsFolder)) { | |||
return $output->writeln('<error>Emoticons folder does not exist</error>'); | |||
$output->writeln('<error>Emoticons folder does not exist</error>'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return is expected here in order to stop the task execution here as the source folder does not exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value inside execute
should return something useful or nothing at all. Returning the output of $output->writeln()
is not related to your command (in fact it doesn't even return anything).
However, I'll add it again (below $output->writeln()
) it since it is indeed required for proper execution.
@helios-ag since I want to update one of my applications, I had to upgrade this bundle to Symfony4, so I did. I made some tiny codestyle changes and fixed few things here and there.
Major points are that the
sensio/distribution-bundle
won't be supported for Symfony4 andsymfony/assetic-bundle
is deprecated. I've just added apublic_path
sincesymfony/assetic-bundle
was only required for it's output path (as far as I could see). Thepublic_path
defaults to the SF4 default for the web folder (which is/public
).If you want me to test anything else or got any feedback, let me know.