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

Check for MD5 changes when deploying static content #4295

Closed
wants to merge 5 commits into from
Closed

Check for MD5 changes when deploying static content #4295

wants to merge 5 commits into from

Conversation

jissereitsma
Copy link
Contributor

The old Publisher is copying files only from their original source to the pub/static folder, if the destination does not exist. This PR changes this, so that for each file the MD5 sum is used as well. Most developers do not want to wipe out files in pub/static when changing file. This fixes that for them. There are other patches underway as well (@denisristic) to add arguments to the deployment command to specify which parts need to be refreshed.

}

return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

return md5_file($source) === md5_file($destination)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually I don't think shortening it will be best. In the future the md5_file() lines might need to be replaced with a wrapped class call (currently $this->filesystem does not support MD5 calls). So I think the explicit call consumes more lines indeed, but allows for better modification in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, the "let's make present just a little bit worse to favour a future which might not come as soon as I think it would" argument. Fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm ;)

@jissereitsma
Copy link
Contributor Author

Travis failed because it inserted a dummy file (/root/something) as source which was not readable. Updated PR now includes also a check for existence of the source file.

@piotrekkaminski
Copy link
Contributor

@jissereitsma Hey. Travis fails on code style checks in your code. Also please make sure to update your fork to latest develop - there were some failures previously caused by our tests.

@jissereitsma
Copy link
Contributor Author

@piotrekkaminski Thanks for notifying me. I've looked into this, and am unable to solve it. The unit test fails with the call to Magento\Framework\Filesystem\Directory\Read::isExists() which seems to suggest something is wrong with my code. Actually it was not: The error was with the rearranged structure, because of which the unit test working with dummy files did not work anymore. I've now restructured the code a bit, and added improved tests to deal with things.

@sprankhub
Copy link
Member

@jissereitsma could you update to latest develop?

@piotrekkaminski could you merge afterwards?

@vrann
Copy link
Contributor

vrann commented Mar 25, 2017

@jissereitsma can you please update this branch with the latest develop?

@denisristic
Copy link
Contributor

denisristic commented Apr 13, 2017

@jissereitsma I've done some testing and I think that I would be much faster (about 10x) to check filemtime than md5_file. If any original file is changed than it would have bigger mtime than the file in static folder and than you could check md5 to see if file is changed.

Here is the output of my test script:

Total files: 34294
Initial ememory usage: 9 mb
Total Execution Time (filemtime): 0.12025809288025 seconds
Peak: 9.06 mb
----------
Total Execution Time (md5): 1.3628039360046 seconds
Peak: 9.07 mb

@okorshenko
Copy link
Contributor

Closing this PR due to inactivity.
Using of md5 is not the best solution in this case. Feel free submit new PR once ready. We will process it faster this time. Today Community Engineering Team is strongly focused on community pull requests processing.

@okorshenko okorshenko closed this Apr 14, 2017
@okorshenko okorshenko self-assigned this Apr 14, 2017
@okorshenko okorshenko added this to the April 2017 milestone Apr 14, 2017
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.

None yet