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

CRM-17652: Make civicrm-packages into a composer library #193

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Jul 25, 2017

This one needs to be merged before civicrm/civicrm-core#10694 can use composer to pull in civicrm-packages and have it actually work (ie. pass tests)

{
"name": "civicrm/civicrm-packages",
"description": "Legacy third party dependencies for CiviCRM",
"type": "library"
Copy link
Contributor

Choose a reason for hiding this comment

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

@dsnopek will we want a mention of composer-installers here given we will need to customise it goes in the civicrm/ folder within library and also shouldn't we have require: { civicrm/civicrm: xxx } ?

@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 6, 2017

@seamuslee001 What I was imagining is that civicrm/civicrm-core would depend on this library - not the other way around. Even though it's a kinda weird library (because it's a bunch of different libraries merged together), it is still PHP code that we want to require, so I think it makes sense that requiring it would end up in the 'vendor' directory (as opposed to use ing composer-installers to put it somewhere else). We would have to adjust the way that civicrm-core finds the packages, though, to look under 'vendor'.

FYI, this bit was based on the plan posted by @totten at https://gist.github.com/totten/0c1a01840ccc13388dbf252f5f9d4893 and it would allow us to drop another line from the process we're trying to eliminate at https://gist.github.com/dsnopek/56311dbea347874e75180883efabb620

@jackrabbithanna
Copy link

@dsnopek this should probably be merged too right? @totten what you say boss?

@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 13, 2017

Yeah, this'll be necessary to eliminate the step from the "temporary" process that copies the packages over.

@seamuslee001
Copy link
Contributor

So the big problem that to me seems to need to be solved is getting this installed in the right folder. I noticed that Tim's gist is still suggesting it is in the packages folder as a sub folder of civicrm.

I'm not sure how to solve this when all other (D7, wordpress, D6 (even still), Joomla) will all still be presuming its in civicrm/packages. I support this and likewise the civicrm-drupal PR. My impression of this would mean at the present moment it would fall in /packages not in /civicrm/packages or is there something else going on?

@dsnopek i agree now with not needing the require here got myself in muddle

@totten
Copy link
Member

totten commented Aug 14, 2017

  • Agree we need composer.json in civicrm-packages.git. We actually need it in all the repos...
  • In the namespace for composer packages, I think civicrm/{foo} makes more sense than civicrm/civicrm-{foo}.
  • We probably should have some discussion about the overall naming/layout of the composer.json files. Filed issue and proposed structure at https://issues.civicrm.org/jira/browse/CRM-21064

@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 23, 2017

re @seamuslee001

So the big problem that to me seems to need to be solved is getting this installed in the right folder.

Conceptually, I think the "right" folder is vendor/civicrm/civicrm-packages (or vendor/civicrm/packages if we go with the naming @totten suggests - I don't have a strong opinion about that) because vendor/ is where 3rd-party PHP library code should go in a composer universe. But that would be a change from the path that the packages are at currently.

If it's possible to move that and adjust the paths used in civicrm-core without breaking backcompat in some way, I think we should have this install under vendor/

If not, well, we can use composer-installers to put it in the old location. But that's something we'd do in the composer.json that depends on this repo (ie. in civicrm-core/composer.json) so that wouldn't require any changes to this PR

@seamuslee001
Copy link
Contributor

Yeh true, in regards to the packages things, So the biggest stuff looking at is CRM/Core/Respources CRM/Core/Smarty and then of course CRM/Core/ClassLoader

There are a number of other places including in status checks and also in upgrade (checking certain files are removed etc etc)

However as far as i can tell all the code is thinking is stuff is either loaded from civicrm/vendor or civicrm/packages (or in d8 the vendor dir straight). So with packages i think if we kept it the same as Tim thinking vendor/civicrm/packages i would think there would be minimal code changes needed then.

Agree with your comments about having it in vendor.

I.e.. looking at stuff like

Civi/Core/Paths.php:      ->register('civicrm.packages', function () {
Civi/Core/Paths.php:          'path' => \Civi::paths()->getPath('[civicrm.root]/packages/'),
Civi/Core/Paths.php:          'url' => \Civi::paths()->getUrl('[civicrm.root]/packages/'),
CRM/Core/Resources.php:      "packages/jquery/plugins/jquery.mousewheel.min.js",
CRM/Core/Resources.php:      "packages/jquery/plugins/jquery.form.min.js",
CRM/Core/Resources.php:      "packages/jquery/plugins/jquery.timeentry.min.js",
CRM/Core/Resources.php:      "packages/jquery/plugins/jquery.blockUI.min.js",
CRM/Core/Resources.php:      "packages/jquery/plugins/jquery.ui.datepicker.validation.min.js",
CRM/Core/Resources.php:      $items[] = "packages/jquery/plugins/jquery.tableHeader.js";
CRM/Core/Resources.php:      $items[] = "packages/jquery/plugins/jquery.menu.min.js";
CRM/Core/Resources.php:      $items[] = "packages/jquery/plugins/jquery.notify.min.js";

its look for civicrm/packages so in theory it doesn't matter wherever you put civicrm as long as there is a packages subfolder code should still work.

@totten i don't see an issue with merging this in its current form

@seamuslee001
Copy link
Contributor

Jenkins test this please

@totten
Copy link
Member

totten commented Feb 9, 2018

As mentioned in the JIRA thread, I flipped from my earlier position -- the civicrm-* prefix should appear in composer package names. It's a little verbose, but it avoids some oddball file-naming. So 👍 on this PR.

@totten totten merged commit 62e9159 into civicrm:master Feb 9, 2018
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 9, 2018

Ah, awesome, thanks! One less thing :-)

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

Successfully merging this pull request may close these issues.

5 participants