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

EZP-25314: Add misc Symfony files, web/css and web/js to .gitignore #75

Merged
merged 1 commit into from
Dec 21, 2015
Merged

Conversation

emodric
Copy link
Contributor

@emodric emodric commented Dec 18, 2015

https://jira.ez.no/browse/EZP-25314

app/check.php and app/SymfonyRequirements.php are (re)generated by Symfony when installing/updating vendors, so they're not too useful to be in the repo.

web/css and web/js folders are also (re)generated when installing/updating vendors, by Assetic.

@andrerom
Copy link
Contributor

+0,9

@bdunogier
Copy link
Member

Hmm, yep, +1.

Could you please create an enhancement for it on JIRA, @emodric ? It is worth having in the changelog.

@emodric emodric changed the title Add misc Symfony files, web/css and web/js to .gitignore EZP-25314 :Add misc Symfony files, web/css and web/js to .gitignore Dec 19, 2015
@emodric
Copy link
Contributor Author

emodric commented Dec 19, 2015

@bdunogier Done!

@yannickroger
Copy link
Contributor

+0,1 With the issue number in the commit message @emodric
You can ping me for a quick merge.

@emodric
Copy link
Contributor Author

emodric commented Dec 21, 2015

@yannickroger Done!

@emodric emodric changed the title EZP-25314 :Add misc Symfony files, web/css and web/js to .gitignore EZP-25314: Add misc Symfony files, web/css and web/js to .gitignore Dec 21, 2015
yannickroger added a commit that referenced this pull request Dec 21, 2015
EZP-25314: Add misc Symfony files, web/css and web/js to .gitignore
@yannickroger yannickroger merged commit 32b2fb4 into ezsystems:master Dec 21, 2015
@yannickroger
Copy link
Contributor

Thanks @emodric

@crevillo
Copy link
Contributor

-0.5 on my side. please not that symfony best practice now recommend to store your css under web/css folder. Same goes for js...
http://symfony.com/doc/current/best_practices/web-assets.html
Edit: seems i arrived here too late anyway...

@yannickroger
Copy link
Contributor

@crevillo you couldn't be faster :)

@yannickroger
Copy link
Contributor

@crevillo Can you create an improvement with what you are pointing out and we'll see of we can fix that.

@crevillo
Copy link
Contributor

well, the point is that symfony best practice tells you to store your css and js directly under web/css web/js.
Traditionally we have put those files in some [bundle]/Resources/public. So, if someony follow that best practice with this .gitignore, it won't be possible to maintain those css with git...

So, ok to +/app/check.php
/app/SymfonyRequirements.php but not really conviced about the other two.

@emodric
Copy link
Contributor Author

emodric commented Dec 21, 2015

I'm not entirelly convinced that Symfony best practices are really best practices in some cases.

With it, views are in app, source and config are in src and css and js are in web.

Too much files being thrown all over the place.

Not to mention that eZ Platform (Demo) is installing and using Assetic which generates to same folders.

@crevillo
Copy link
Contributor

Well ,but still those are their best practices... We can discuss them, of course, but still are their best practices.
And btw, even in eZ Platform assetic is ussed working files to css nothing forbides you to add your own css on those folders too... so... We don't have a rule telling devs "don't put your css here". so still -0.5 :)

Edit. About the web/css and web/js regeneration, the only problem a dev could have is

  1. He is using output filter to output the compiled css to a file having the same as one of their css
  2. By casualliy, one of their css is called something like [number]_styles.css or whatever.

Afaik, by default assetic adds those numbers to css generated in dev ...

@emodric
Copy link
Contributor Author

emodric commented Dec 21, 2015

Sure, but using folders where something autogenerates files for your own custom code is asking for trouble IMO.

You can kiss goodbye being able to just delete the folders and recreate them from scratch by running assetic:dump for example.

EDIT: And Symfony does not include Assetic by default any more. That's why storing assets under web/css and js/css makes sense with Symfony. So eZ Platform should either stop shipping Assetic, or make their own best practices.

@emodric
Copy link
Contributor Author

emodric commented Dec 21, 2015

As i mentioned on Slack, using Symfony best practices for the sake of using them is wrong, if they don't fit.

@crevillo
Copy link
Contributor

Well, but that's your opinion. Mine is this best practice still fits.. There's nothing prohibiting ez developers put assets under web folder. There's nothing prohibiting symfony devs either. Reading why this best practice has been recommended by symfony, there are also good reasons for.

what we are doing here is maybe a commodity in terms of better managing our git repositories, but if me or any other would like to add assets under web we will to modify these .gitignore in order to have them versioned i would like to do it and not worrying about this .gitignore. It will be my responsibility to take care of assetic stuff...

Not to mention we're adopting more and more symfony ways or doing things. We did that with our coding standards, right? Also The goal is to attract more developers and make them easier to begin with ez devs, why we are going to tell them "hey, this is symfony but don't apply this best practice you're applying for any of your projects"?

Not to mention also that you are not obliged to use assetic either.... so...

@emodric
Copy link
Contributor Author

emodric commented Dec 21, 2015

This is not a matter of personal preference. I can always add (and I do add) those folders to .gitignore, just as you can remove them.

And I agree that eZ Platform should attract more Symfony developers, but this is still a matter of mixing apples and oranges, though.

It's simple: Assetic uses those folders for its generated files, so it basically reserves those folders for it's internal use. Using them outside those constraints is asking for trouble in my opinion.

The right course of action would be to:

  • either remove Assetic from distribution (as Symfony did), in which case Symfony best practice would make sense
  • Or keep Assetic, but make these files ignored by Git because they are reserved by Assetic.

@bdunogier
Copy link
Member

Or make assetic use another folder for assetic ? I mean, it's a bot, it doesn't care where it writes things. web/js and web/css are clear, usable directories. ezplatform-demo uses web/assets/css. I'd rather remove the "assets" part :-)

@emodric
Copy link
Contributor Author

emodric commented Dec 21, 2015

@bdunogier If it can be configured, then great :) I've never looked TBH.

@crevillo
Copy link
Contributor

sorry, but they aren't. you can still add css to web/css, use assetic and it will create new files without touching yours. you can also configure to write them wherever you want

I don't see the point on deleting assetic from eZ Platform. Indeed Symfony has removed but assetic is still considered as a good practice by Symfony. It's said in the same url. Removing assetic from ezplatform distro will imply a lot of rework on platform ui, if i'm not wrong

Also, assetic was removed since 2.8. Putting assets under web is best practice since 2.7.
http://symfony.com/doc/2.7/best_practices/web-assets.html

@emodric
Copy link
Contributor Author

emodric commented Dec 21, 2015

That's right, you can. But sooner or later someone will loose some files and report issues to eZ Systems in line of: "Assetic deleted my CSS and JS files".

Ideally Assetic should have a different path other than web/css web/js

@bdunogier
Copy link
Member

I don't see why assetic would remove any file. It's not names it knows about, and I haven't seen it remove anything so far.

But yes, the folders should be separated, or it'll be a mess. Tested with the following, it works:

framework:
  assets:
    base_path: 'compiled/'

assetic:
  # default is %kernel.root_dir%/../web
  write_to: '%kernel.root_dir%/../web/compiled'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants