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

Static deploy flags #4294

Merged
merged 17 commits into from
Aug 17, 2016
Merged

Conversation

denisristic
Copy link
Contributor

@denisristic denisristic commented Apr 25, 2016

When calling setup:static-content:deploy now you are able to put flags to skip specific content and you are able to do that for specific theme(s)

Usage:
setup:static-content:deploy [-d|--dry-run] [--no-javascript] [--no-css] [--no-less] [--no-images] [--no-fonts] [--no-html] [--no-misc] [--no-html-minify] [-t|--theme[="..."]] [-et|--exclude-theme[="..."]] [-l|--language[="..."]] [-el|--exclude-language[="..."]] [-a|--area[="..."]] [-ea|--exclude-area[="..."]] [languages1] ... [languagesN]

Options:
--dry-run (-d) If specified, then no files will be actually deployed.
--no-javascript If specified, no JavaScript will be deployed.
--no-css If specified, no CSS will be deployed.
--no-less If specified, no LESS will be deployed.
--no-images If specified, no images will be deployed.
--no-fonts If specified, no font files will be deployed.
--no-html If specified, no html files will be deployed.
--no-misc If specified, no miscellaneous files will be deployed.
--no-html-minify If specified, just html will not be minified and actually deployed.
--theme (-t) If specified, just specific themes will be deployed (multiple values allowed)
--exclude-theme (-et) If specified, specific theme will not be deployed (multiple values allowed)
--language (-l) If specified, just specific language will be deployed (multiple values allowed)
--exclude-language (-el) If specified, specific language will not be deployed (multiple values allowed)
--area (-a) If specified, just specific area will be deployed (multiple values allowed)
--exclude-area (-ea) If specified, specific area will not be deployed (multiple values allowed)

Denis Ristic added 5 commits April 25, 2016 16:08
…s (js, css, less, images, fonts...) and filter by theme""

This reverts commit 04ebbe796c2a11608581fecdf4d9b6645e19d2e1.
…filetypes (js, css, less, images, fonts...) and filter by theme"""

This reverts commit 8216e12.
…ss, less, images, fonts...) and filter by theme"

This reverts commit 4e65486.
…, images, fonts...) and filter by theme - fixed
return true;
} else if ($this->isFonts && in_array($ext, array('eot', 'svg', 'ttf', 'woff', 'woff2'))) {
return true;
} else if ($this->isMisc && in_array($ext, array('md', 'txt', 'jbf', 'csv', 'json', 'txt', 'htc', 'swf', 'LICENSE', ''))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use elseif instead of else if, or else if "so that all control keywords look like single words".

Copy link
Contributor

@adragus-inviqa adragus-inviqa Apr 25, 2016

Choose a reason for hiding this comment

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

I would rather you put those file extension strings somewhere - if not configurable - at least more visible, like some private properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for your remarks

@adragus-inviqa
Copy link
Contributor

Please use PHP 5.4's array notation - [] - instead of array. It's less noisy that way.

@mazhalai
Copy link
Contributor

mazhalai commented May 9, 2016

@denisristic thank you for your contribution, we have created MAGETWO-52614 to process this PR.

@mazhalai mazhalai added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label May 9, 2016
@piotrekkaminski
Copy link
Contributor

@denisristic can you sign the contributor license agreement? it is stopping us from merging this nice improvement.

@denisristic
Copy link
Contributor Author

@piotrekkaminski I've signed it several times but back then it didn't change status. As I can see now it's changed even though I haven't done anything

@hostep
Copy link
Contributor

hostep commented May 31, 2016

@denisristic: I happen to notice you changed the file permissions of the two php files from 644 to 755, I think you'll need to put them back to 644.

Other than that, @ Magento team: do you think this PR will be included in Magento version 2.1?
I haven't tested it yet, but it looks like the --themes switch will allow us to significantly improve the time for each deployment on a production server.

@hostep
Copy link
Contributor

hostep commented May 31, 2016

Just did some testing using this patch, in case anyone is interested (I'm running in production mode btw):

# First remove all caches
$ rm -R pub/static/*
$ rm -R var/cache/*
$ rm -R var/page_cache/*
$ rm -R var/generation/*
$ rm -R var/view_preprocessed/*

# Now run static deploy without a filter
$ time php bin/magento setup:static-content:deploy

...

New version of deployed files: 1464678133

real    6m35.824s
user    6m24.147s
sys     0m14.243s


# Now do the same, first removing all caches again
$ rm -R ...

# Run static deploy with filter on backend theme and our custom theme
$ time php bin/magento setup:static-content:deploy --themes Magento/backend --themes Vendor/custom

...

New version of deployed files: 1464678974

real    2m32.092s
user    2m23.732s
sys     0m5.688s

Over 2 times faster, wow!

Disclaimer: I have 2 custom themes set up, so the first run should be faster if you only have 1 custom theme.

Thanks @denisristic, awesome work!

Hope this gets merged in Magento 2.1 codebase.

@erikhansen
Copy link
Contributor

erikhansen commented Jun 1, 2016

@denisristic Thanks for working on this PR. Two suggestions/requests:

  1. You may want to rename your --themes flag to --theme since it looks like you have to pass separate arguments for each theme you want to include. Alternatively you could rework your --themes flag to accept a comma-delimited list of themes. For example: bin/magento setup:static-content:deploy --themes=vendor/theme1,vendor/theme2
  2. Per my comment on a issue Skip Luma theme when compiling static resources #3038, it would be great if you could add an --exclude-theme flag that would allow a developer to specify which themes NOT to include. This would be especially useful for a Magento site where new themes might be added over time and the merchant is using deployment scripts that have hard-coded deployment steps. For example: bin/magento setup:static-content:deploy --exclude-theme=magento/luma --exclude-theme=magento/blank

@erikhansen
Copy link
Contributor

@denisristic Are you interested in adding a --exclude-theme flag as I suggested in my previous comment? If not, let me know.

@piotrekkaminski Can you confirm that you're intending to merge this PR once the current merge conflicts are resolved and it goes through code review?

@denisristic
Copy link
Contributor Author

@erikhansen Thanks for your comments. I`ll add exclude flag version for themes until the end of the week.

@piotrekkaminski
Copy link
Contributor

@erikhansen yes - it is very high in the backlog for next version (2.2), however won't be possible for 2.1.

@giacmir
Copy link
Member

giacmir commented Jun 14, 2016

It would be nice to have a switch to automatically grab all configured languages without need to explicitly pass them as arguments. In case of installations with several languages it is pretty easy to forget one, and it is very useful for automated deploys.

@erikhansen
Copy link
Contributor

@denisristic Any update on adding the --exclude-theme flag and resolving the merge conflicts in this branch so that this PR can be merged in?

ADDDED --exclude-theme flag (@erikhansen suggestion)
UPDATED --language flag (@giacmir)
ADDED flags: --area (-a), --exclude-area (-ea), --exclude-language (-el), --exclude-theme (-et)
ADDED flag shotcuts: -l, -el, -t, -et, -a, -ea
ADDED check for wrong language, theme and/or area parameter value
FIXED variable names, changet do camelCase
FIXED Invalid argument supplied for foreach()
FIXED code style (PHPCS & PHPMD)
@mslabko mslabko self-assigned this Jun 30, 2016
@mslabko
Copy link
Member

mslabko commented Jun 30, 2016

Hi @denisristic
Could you please sign the License Agreement? https://cla.dev.magento.com/magento/magento2?pullRequest=4294

@denisristic
Copy link
Contributor Author

denisristic commented Jul 5, 2016

Hi @mslabko ,
I've done it at least 20 times but it didn't change.
I can see that it is signed now even though I didn't do anything in last 10 days

@mslabko
Copy link
Member

mslabko commented Jul 11, 2016

@denisristic ,
Thank you! Your PR already in progress.

@vkorotun vkorotun removed the PS label Aug 3, 2016
@vkorotun vkorotun added improvement and removed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development labels Aug 5, 2016
@sshrewz sshrewz added the linked label Aug 11, 2016
@sshrewz
Copy link

sshrewz commented Aug 11, 2016

@denisristic, we need you to sign the CLA before we can proceed with further actions your contribution.

@adragus-inviqa
Copy link
Contributor

@denisristic, we need you to sign the CLA before we can proceed with further actions your contribution.

Not only did @denisristic say that he signed the thing 10 times, but I'm seeing it signed already. Fix the signing process already.

@denisristic
Copy link
Contributor Author

@Snohe I've signed it at least 20x, there is some bug with signing process. As I can see now it is signed even though I didn't do anything

@mslabko
Copy link
Member

mslabko commented Aug 12, 2016

@Snohe PR already in progress, CLA is applied.

@mmansoor-magento mmansoor-magento merged commit 6766de7 into magento:develop Aug 17, 2016
@mslabko
Copy link
Member

mslabko commented Aug 17, 2016

@denisristic , PR was merged to mainline. Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

Successfully merging this pull request may close these issues.