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

Add condition to avoid warning on calling file_exists() #2176

Closed
wants to merge 1 commit into from
Closed

Add condition to avoid warning on calling file_exists() #2176

wants to merge 1 commit into from

Conversation

jacquesbh
Copy link

fixes #2128

The warning was: "Warning: file_exists(): File name is longer than the maximum allowed path length on this platform (4096)…"
The cause was: if we give the schema content instead of the schema filename as the second parameter of validateDomDocument
we get this error if the schema is more longer than 4096 chars.

fixes #2128

The warning was: "Warning: file_exists(): File name is longer than the maximum allowed path length on this platform (4096)…"
The cause was: if we give the schema content instead of the schema filename as the second parameter of validateDomDocument
we get this error if the schema is more longer than 4096 chars.
@@ -274,7 +274,8 @@ public static function validateDomDocument(
$schema = self::$urnResolver->getRealPath($schema);
libxml_use_internal_errors(true);
try {
if (file_exists($schema)) {
// 4096 is the maximum length allowed by file_exists for the filename
if (strlen($schema) < 4096 && file_exists($schema)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code will be removed, so the fix will not be required.

Copy link
Author

Choose a reason for hiding this comment

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

Yes ok. But actually it doesn't work. So… If I put myself at the same point of view as you… Why not merging this and removing the code after? You just said you will remove it. But fixing problems until then is still a good idea, isn't it?

Seems to be a good idea, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean it's already removed and will be published with next publication. Merging this PR will just cause extra conflict.

@orlangur
Copy link
Contributor

if we give the schema content instead of the schema filename as the second parameter of validateDomDocument

How can this happen?

@jacquesbh
Copy link
Author

@orlangur Look at this commit: f3a12f2
And this issue: #2128

So, if the problem is solved (I have conflicts with this PR… which changes only one line…), maybe the main problem which is "sending content of an XML file to file_exists()" is still there?

@orlangur
Copy link
Contributor

@jacquesbh, I've looked into them before my comment. By my question I meant "what is the situation when such error occur except for human error?". As obviously you can put everything into everything and adding checks for all such cases is not really necessary.

Now I understood this method was polymorhic and accepted not just schema file name but source as well.

If you look into mainline, there is no more check for file presence: https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/Config/Dom.php#L290 Thus I believe problem disappeared by design and your fix is not applicable (but it fixed a real flaw overlooked in initial implementation, not just human error case).

@davidalger davidalger closed this Nov 10, 2015
@jacquesbh jacquesbh deleted the bugfix/MAGETWO-44227-xml-source-instead-of-xml-filename branch November 20, 2015 00:19
magento-engcom-team pushed a commit that referenced this pull request Mar 7, 2018
[Plankton] Prepare codebase for 2.2.4
@ghost ghost mentioned this pull request Mar 15, 2019
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.

4 participants