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

Fix EZP-22991: Added file exists control for parse method #888

Closed

Conversation

dymissy
Copy link
Contributor

@dymissy dymissy commented Jun 5, 2014

After update to 2014.03 version, when functional tests are launched the path of tr files is wrong. This PR add a file exists check before to launch the parseString method.

This is not the better solution but I'm not sure if I can thrown an Exception (if so which one?) and, above all, if I can fix the path directly in the parse method. I don't know if this method is used elsewhere and if in other cases the path works fine.

Any suggestions in order to fix the issue are more than welcome.

@ezrobot
Copy link
Contributor

ezrobot commented Jun 5, 2014

This Pull Request does not respect our PHP Coding Standards, please, see the report below:

FILE: ...ish/Core/Persistence/TransformationProcessor/DefinitionBased/Parser.php
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 90 | ERROR | Expected "if (...)\n...{\n"; found "if(...)\n...{\n"
 90 | ERROR | A space is required after opening parenthesis of function call
 90 | ERROR | A space is required before closing parenthesis of function call
 94 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

@andrerom
Copy link
Contributor

andrerom commented Jun 6, 2014

I would opt for NotFoundException for this case, but ping @pspanja for second opinion.

@dymissy
Copy link
Contributor Author

dymissy commented Jun 6, 2014

Thanks @andrerom!

👍 for the Exception but I'm not sure if NotFoundException is the better choice. This is an example of the output returned with this class:

Could not find 'file' with identifier 'eZ/Publish/Core/Persistence/Tests/TransformationProcessor/_fixtures/transformations/ascii.tr'

I think it is developed for contents more than for files but I'd like to know your point of view.

@pspanja
Copy link
Contributor

pspanja commented Jun 9, 2014

@andrerom @dymissy I would rather throw RuntimeException, as this is something that should IMO crash the app. But we should also fix the problem, and I'm not sure how to reproduce this?

@bdunogier bdunogier closed this Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants