?

Success

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
19 May 2014

Part of the vision for 3.4 is integrating Composer into the CMS. This pull request as of this time represents a working solution for this. Changes with this pull request:

  • Composer installed code is under libraries/vendor
  • Third party code which we have manually added that is installable via Composer is handled as such (Symfony YAML component, the password_compat library, PHPMailer, and lessc)
  • Framework code we have manually imported is moved into the Composer installation
  • The Framework's DI package is updated to 1.2 (we're shipping 1.0 as of 3.3)
  • The Platform's Input package is replaced with Joomla\Input from the Framework with class mapping for B/C (same as what was done for the Registry package)
  • The build script is modified to only package production files from the Composer installed packages
  • JApplicationBase now extends Joomla\Application\AbstractApplication and now duplicated code or class variables in that class and child objects are cleaned up

TODO:

  • Verify the build script is working completely as intended for all package types
  • Determine if unused Framework packages (brought in because of dependencies within the Framework code) should be packaged with releases; if so then we should look at perhaps using it and removing the Platform equivalent otherwise we should ensure the build script excludes those files

Votes

# of Users Experiencing Issue
0/1
Average Importance Score
5.00

avatar mbabker mbabker - open - 19 May 2014
avatar wilsonge
wilsonge - comment - 20 May 2014

OK so I've PR'd against the Framework Application branch to remove the event dependency like we discussed a while ago but never did - so that should remove that from the PR. Also PR'd input to update copyrights there to 2014.

Finally I there have been some minor changes but I think we can map JArrayHelper - but there have been some minor changes so we'd need to make sure there are no b/c breaks there - I only glanced over.

avatar mbabker
mbabker - comment - 20 May 2014

I looked at it. Typehinting, protected methods being renamed and declared private; can't happen.

avatar wilsonge
wilsonge - comment - 20 May 2014

OK so are we gonna make the same hack to the registry package or we gonna have two classes doing the same thing with the same public methods :P

avatar mbabker
mbabker - comment - 20 May 2014

You like to mention B/C and whatnot, so out of the dev strategy:

All PHP code in the /libraries folder which is not flagged as private is considered to be part of the Joomla API and subject to backwards compatibility constraints.

Protected methods can't be hacked up like that.

avatar wilsonge
wilsonge - comment - 20 May 2014

So do we do the hack to the framework registry package we did in the cms for 3.2? or do we have two classes?

avatar mbabker
mbabker - comment - 20 May 2014

Are you going to take accountability for reapplying that hack every time a composer update is run on the CMS? Leave it as it is, there isn't much that can be done about it now.

avatar wilsonge
wilsonge - comment - 20 May 2014

I try my level headed best not to take accountability for anything :D

avatar mbabker
mbabker - comment - 20 May 2014

I rest my case, Your Honor. :-D

avatar b2z
b2z - comment - 20 May 2014

What can I say? Only wow, the huge job is done! And so many files changed - how to review the code? :)

@mbabker - are there any specific tasks for testers?

avatar infograf768
infograf768 - comment - 20 May 2014

Travis fails, but other than that, can we separate the phpmailer update from this to make testing easier.

avatar Bakual
Bakual - comment - 20 May 2014

Showing 224 changed files with 30,106 additions and 5,052 deletions.

I agree that it would be nice if we can split that up a bit. 224 changed files is a huge list.
Also it looks to me like composer pulls in a lot of unneeded stuff. ~25'000 added lines to our codebase? Wow.

PhpMailer seems to pull in about 90 files. While we currently only have 7 in our repo. Looks like we handle the language differently and we obviously don't need the docs, example and extras folder. I'm also not sure about the test folder, I don't think we need those?
I don't know composer enough yet. Does it allow to only download what we need or does it always download the full package?

avatar piotr-cz
piotr-cz - comment - 20 May 2014

@Bakual , no composer doesn't allow that. A solution would be to have a lightweight version of PHPMailer, or add an post-package-update script to composer.json that would remove unnecessary files.

avatar infograf768
infograf768 - comment - 20 May 2014

Did not look in details, but I also see that phpmailer contains quite a few php language files in libraries/vendor/phpmailer/phpmailer/language/

Have you tested that libraries/joomla/mail/language/phpmailer.lang-joomla.php overrides these and that we can keep the strings in the xx-XX.ini file

avatar wilsonge
wilsonge - comment - 20 May 2014

We also still have the cli classes living in /framework/joomla/application which we should remove. Otherwise I guess we're gonna have class/namespace names clashing?

avatar mbabker
mbabker - comment - 20 May 2014

Doing this all in one shot so bear with me please.

@b2z wrote:

@mbabker - are there any specific tasks for testers?

Just make sure the CMS works properly with the core setup and your favorite extensions :smile:

@infograf768 wrote:

Travis fails, but other than that, can we separate the phpmailer update from this to make testing easier.

There's a PR to the branch for the unit tests I haven't gotten to look at yet but that should be fixed shortly. As for the PHPMailer update, I'd actually suggest someone propose a separate PR updating PHPMailer for 3.3.1 and that would take care of that issue.

@Bakual wrote:

Does it allow to only download what we need or does it always download the full package?

As @piotr-cz pointed out, Composer itself will clone a git repo's tag (and if there's a .gitattributes file, exclude files that are listed in there as excluded when doing so, see this example), so it always downloads the full repo. We can script it to dump all the unnecessary fluff, but I'm personally not a fan of touching anything in the vendor folder once it's downloaded (most projects gitignore it, that's not exactly something we can do right now). It would sure make managing the build script updates easier though...

@infograf768 wrote:

Have you tested that libraries/joomla/mail/language/phpmailer.lang-joomla.php overrides these and that we can keep the strings in the xx-XX.ini file

Yes, our language file is still used. The other files being present is part of that extra fluff @Bakual commented about.

avatar wilsonge
wilsonge - comment - 20 May 2014

Unit Test Updates:
So I've fixed some unit tests but there are still some issues outstanding

https://github.com/joomla-framework/registry/blob/master/src/Registry.php#L310 This is currently being typehinted but should return false if we don't merge a instance of JRegistry.

There's also 4 JInput tests failing - not so sure about why. Someone probably is going to need to take some time to go through them. The Cli one looks like a change in expected behaviour so might be problematic. The other 3 are more serious bugs but my guess is they should be trivial to fix

avatar mbabker
mbabker - comment - 20 May 2014

There were a handful of bug fixes applied to the Input package in the Framework. That might be why the tests in the CMS are failing.

avatar sybrek
sybrek - comment - 20 May 2014

I really don't like the libraries/vendor directory. It just feels wrong. If i'm not alone with this feeling, i would recommend to introduce a new directory-constant (JPATH_VENDOR ?) to make future changes easier.

avatar wilsonge
wilsonge - comment - 20 May 2014

That was my idea because I don't like not having libraries within the libraries folder..... :P We have a CMS directory-constant JPATH_LIBRARIES which is exactly for that..... I would have put it there except unfortunately we then mix up namespaced and non-namespaced code in the joomla folder in libraries. So the compromise was libraries/vendor

avatar mbabker
mbabker - comment - 21 May 2014

I don't like it either. The two arguments given were that libraries thing and introducing another top level folder (vendor) adds another name that CMS users cannot use for their top level menus (and vendor is something that's potentially in use versus when we added the bin or cli folders).

avatar b2z
b2z - comment - 21 May 2014

i would recommend to introduce a new directory-constant

I feel that that we do not need more constants.

avatar sybrek
sybrek - comment - 21 May 2014

another name that CMS users cannot use for their top level menus

So why not using capitals (e.g. Libraries) like everybody else ? This would be a possibility to differ the new structure (which we will need) from the old source.

avatar Bakual
Bakual - comment - 21 May 2014

So why not using capitals (e.g. Libraries) like everybody else ? This would be a possibility to differ the new structure (which we will need) from the old source

Afaik this would only work on Unix/Linux systems, not on Windows.

avatar sybrek
sybrek - comment - 21 May 2014

Is IIS still not case sensitive ? I thought they wanted to change that.

avatar piotr-cz
piotr-cz - comment - 21 May 2014

@sybrek I don't know about IIS, but PHP on windows is using it's filesystem which is case insensitive. At least on WAMPs

avatar Bakual
Bakual - comment - 21 May 2014

It's Windows which isn't case sensitive. I don't think IIS or any other application could change that, otherwise the Windows Explorer would have real problems accessing the files :smile:

avatar sybrek
sybrek - comment - 21 May 2014

Okay, never mind. I still dont like libraries/vendor ;) Maybe we will find a better solution in the future.

avatar HermanPeeren
HermanPeeren - comment - 21 May 2014

Only Linux is case sensitive, but that is not the question here. For there is no need to change /libraries to /Libraries: Libraries or Vendor is not part of the namespace. Joomla's /libraries is just the same as the folder /vendor that is used by composer. I like /libraries more than /vendor, because /vendor is so much associated with selling something. But because the Symfony guys were among the first people to PHP-FIG workgroup they put that Symfony-convention to use /vendor into the Composer installer and we are more-or-les stuck with it.

BUT: we can make a custom installer-plugin for composer. It's only a few lines of code. See: https://getcomposer.org/doc/articles/custom-installers.md . Then we can load everything to our good old /libraries instead of to /vendor. The rest stays the same (I hope).

avatar sybrek
sybrek - comment - 21 May 2014

BUT: we can make a custom installer-plugin for composer. It's only a few lines of code. See: https://getcomposer.org/doc/articles/custom-installers.md . Then we can load everything to our good old /libraries instead of to /vendor. The rest stays the same (I hope).

As is understand, the current "Composer Integration" is only about core libraries. And the use of /libraries/ brings the problem which @wilsonge has mentioned.

avatar wilsonge
wilsonge - comment - 21 May 2014

Actually composer does already allow custom paths without any custom installer-plugins (if you look that's why it's currently going to libraries/vendor https://github.com/joomla-projects/joomla-cms/blob/ComposerIntegration/composer.json#L9

In Joomla 4 my personal vision is that composer moves upto libraries and we bung all non-framework libraries into the CMS folder (and all deprecated classes into legacy). But for the time being I think libraries/vendor keeps a good line of keeping what we already have (JPATH_LIBRARIES etc.).

avatar b2z
b2z - comment - 21 May 2014

In Joomla 4 my personal vision is that composer moves upto libraries and we bung all non-framework libraries into the CMS folder (and all deprecated classes into legacy). But for the time being I think libraries/vendor keeps a good line of keeping what we already have (JPATH_LIBRARIES etc.).

:+1:

avatar elkuku
elkuku - comment - 21 May 2014

My "vision" would be to have a www folder or similar, containing the "web root".

I believe this would immediately solve all the problems mentioned here :wink:

avatar b2z
b2z - comment - 21 May 2014

My "vision" would be to have a www folder or similar, containing the "web root".

Remember it when we will discuss Joomla 4 ;)

avatar piotr-cz
piotr-cz - comment - 21 May 2014
avatar wilsonge
wilsonge - comment - 22 May 2014

I was just looking at the drupal 8.x code earlier today and noticed they had done the same thing as us - put their composer things in core/vendor instead of just in vendor. So it's not like we're going to be unique by using libraries/vendor

avatar mbabker
mbabker - comment - 28 May 2014

Thinking a bit the last couple of days on the "bloat code" piece, I think we can safely do a post-install script interfacing with Composer to drop out the non-production/distro stuff (tests, JSON/XML files, etc.). But we need to ensure that the code we keep is working.

avatar bweston92
bweston92 - comment - 30 Jun 2014

Any update on this?

avatar wilsonge
wilsonge - comment - 6 Aug 2014

Summary

The cli tests need joomla-framework/input#11 to be pulled

The JInput Tests are failing because of joomla/joomla-framework@58dc9f9 which is technically a b/c issue. Not quite sure how we deal with it.

The registry is failing because we typehint in the framework and we don't in the CMS.

We also need to look at removing the framework event and session packages as they aren't compatible and are just bloat.

avatar nicksavov nicksavov - change - 21 Aug 2014
Labels Removed: ?
avatar johanjanssens
johanjanssens - comment - 30 Aug 2014

@mbabker Quick question. Will the composer folder by /vendor or /libraries/vendor for Joomla 3.4 ? Just want to ensure our Joomlatools Composer Installer is forward compatible with 3.5.

avatar mbabker
mbabker - comment - 30 Aug 2014

The preference is libraries/vendor and that's what the current PR code is supporting.

avatar mbabker
mbabker - comment - 30 Aug 2014

Having not been here for a minute, I have a thought about the concerns from some regarding "bloat code". Unless we're going to hack up what's in the vendor folder (we shouldn't), everything that's in there needs to work otherwise it looks bad on us for shipping what's supposed to be production quality code but doesn't include all its dependencies or properly function. I do realize that 99% of users won't even care, but that's my opinion on the matter.

avatar johanjanssens
johanjanssens - comment - 30 Aug 2014

@mbabker Is there a specific reason why vendor per PR is moved to /libraries/vendor ? If not, wouldn't it make sense to just use the default /vendor ?

avatar mbabker
mbabker - comment - 30 Aug 2014

See this PR for that discussion.

avatar johanjanssens
johanjanssens - comment - 30 Aug 2014

Thanks @mbabker. Will adapt our installers to follow.

avatar brianteeman brianteeman - change - 30 Aug 2014
Status New Pending
avatar stevenrombauts stevenrombauts - reference | - 30 Aug 14
avatar mbabker
mbabker - comment - 3 Sep 2014

I've built packages based on 5b111ce and uploaded them at http://developer.joomla.org/PR-packages/3617/ to give an idea on what the production package will look like with this PR. I know there's still a lot of bloat in the repo, but something we need to deal with for the moment IMO. We might be able to add a lot of that stuff to the .gitignore, but this should not break any of the packages in the repo.

avatar mbabker
mbabker - comment - 3 Sep 2014

We've made a change in the Framework's Registry package to support the CMS, but that change will only apply to future Framework 1.x packages; it was immediately reverted in the 2.0 branch. I've updated the packages at http://developer.joomla.org/PR-packages/3617/ to commit 88cfda4 which includes the update.

The last "blocker" is how to deal with the Input package. An option is to keep our concrete JInput class overriding the Joomla\Input\Input constructor to keep the data source as is for the 3.x series (deprecated for 4.0 to adapt the Framework's changed behavior). Thoughts?

avatar mbabker
mbabker - comment - 4 Sep 2014

@wilsonge a1c8146 should buffer as a B/C layer for the JInput change.

avatar mbabker
mbabker - comment - 4 Sep 2014

Actually, that's not going to be an adequate solution. All typehinting to JInput fails now if the object was a sub-class since everything extends the namespaced class.

avatar wilsonge
wilsonge - comment - 4 Sep 2014

Plus it still doesn't actually fix the unit tests :P

avatar wilsonge
wilsonge - comment - 11 Sep 2014

I just gave this a spin "as is" and things are looking good other than a fatal thrown by us having two instances of php-utf8 library (one in string package in framework, one in core). I fixed that by hacking a bit and all seemed to work well

avatar mbabker
mbabker - comment - 17 Sep 2014

To address the concern with potentially two versions of the phputf8 library functions being loaded in different places by core, I've extended JString from the Framework's String class which is basically the same code (plus some new methods). I've also mapped the Inflector and Normalise classes to the Framework counterparts since they were 100% compatible.

avatar wilsonge
wilsonge - comment - 18 Sep 2014

Unit tests are now failing after the JString changes :P

avatar wilsonge
wilsonge - comment - 18 Sep 2014
avatar mbabker mbabker - change - 22 Sep 2014
Milestone Added:
avatar mbabker mbabker - change - 22 Sep 2014
Milestone Added:
avatar mbabker
mbabker - comment - 23 Sep 2014

Updated test packages are posted to http://developer.joomla.org/PR-packages/3617/ in sync with commit b6031f9

avatar betweenbrain
betweenbrain - comment - 24 Sep 2014

An initial test of http://developer.joomla.org/PR-packages/3617/Joomla_3.4.0-dev-Development-Full_Package.zip, with sample data, seems to install and work fine.

avatar betweenbrain
betweenbrain - comment - 24 Sep 2014

Joomla 3.3 updated with http://developer.joomla.org/PR-packages/3617/Joomla_3.4.0-dev-Development-Update_Package.zip seems to work fine after the update. Tested with some 3PD extensions and all seem to work as well.

avatar wilsonge
wilsonge - comment - 2 Oct 2014

@test I've been building a new website template on this branch and everything seems to be good. no issues in things like akeeba backup or some of no numbers extensions + tested the majority of core extensions without issues.

I reckon we're probably good to merge this.

avatar Fedik
Fedik - comment - 3 Oct 2014

have question,
as I understand currently this only for build the CMS, or?
how it will be for extension developers, to install additional libraries? or this not planned currently?

avatar johanjanssens
johanjanssens - comment - 3 Oct 2014

@Fedik For extension developer who wish to install extensions in Joomla you can check out our Joomlatools Composer installer plugin. The plugin is capable of installing any type of extension into Joomla.

Github repo can be found here https://github.com/joomlatools/joomla-composer and developer docs are here : http://developer.joomlatools.com/tools/composer/introduction.html

avatar mbabker
mbabker - comment - 4 Oct 2014

@Fedik - At the moment, this is only affecting the main CMS build. The intent is to further the integration to allow developers to use it, but right now that isn't coded.

All - Updated test packages are posted to http://developer.joomla.org/PR-packages/3617/ in sync with commit c9d22b9

avatar pete-rossetti
pete-rossetti - comment - 4 Oct 2014

Installed the update package on the latest staging version. Initially got a white screen on update then after a couple of refreshes it confirmed install successful.
Update from web button does not work, in extension manager
Tested 3.4.0dev by installing JEvents, Chronoforms, Phocodownlods and Virtumart 2.9 successfully

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3617.

avatar sandstorm871
sandstorm871 - comment - 4 Oct 2014

I have tested a clean install of 3.3.6 staging on MAMP server using PHP5.5.10
Then used the 3.4dev update package & all seemed to update OK.
Installed a handful of components including JCE, Akeeba admin tools & backup, Sh404 & Virtuemart 2.9.9 ;) all seems to work OK.
I did notice that "Install from web" button doesn't work


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3617.

avatar smehrbrodt
smehrbrodt - comment - 8 Oct 2014

Is there a reason you add the libraries/vendor folder to git?
Wouldn't it make more sense to exclude it from git and just include them in the release builds?
(And add a note for those who check out from git to do a composer install after checkout?

That's how Composer is usually used AFAIK.

avatar Bakual
Bakual - comment - 8 Oct 2014

Wouldn't it make more sense to exclude it from git and just include them in the release builds?
(And add a note for those who check out from git to do a composer install after checkout?

It's one way you could do it, but it makes it harder for people to test using a Git repo as they need composer installed and need a bit of knowledge about how to use it.
It also makes the build process more complex and adds risks that it fails due to server not reachable or similar. Travis also would always have to do the composer stuff before running unit tests.

I don't see any drawback having the vendor folder in the repo under version control.

avatar mbabker
mbabker - comment - 8 Oct 2014

@Bakual covers some of the logic well. The primary thought at this point is that there is a much higher level of skill needed to be able to use the CMS from the source repo if we required everyone to be able to download and install Composer locally. The build scripts or Travis issues aren't as big of points IMO; everyone is dealing with that today (including myself in other projects) and I'd say Composer's caching works well unless you're installing never-before-used code on a platform (or using a platform which doesn't cache such as Travis). FWIW, Drupal 8 also includes their core/vendor folder in version control.

avatar mbabker
mbabker - comment - 16 Oct 2014

Updated test packages are posted to http://developer.joomla.org/PR-packages/3617/ in sync with commit 26f09fa

avatar nicksavov nicksavov - change - 16 Oct 2014
Labels Added: ?
avatar haydenyoung
haydenyoung - comment - 17 Oct 2014

@test

Successful test of core installation and update from 3.3.x.
Successful test install and configure Kunena
Successful test install and configure JSpace (installer installs plugins + modules)
Successful test install of custom template

Also tested installing 3rd libs via composer. It deleted contents of libraries/vendor but installed successfully.

Installed Aws S3 via custom composer.json to libraries/vendor. Was able to register namespaces and use libs:

JLoader::registerNamespace('Aws', JPATH_LIBRARIES.'/vendor/aws/aws-sdk-php/src/');
JLoader::registerNamespace('Guzzle', JPATH_LIBRARIES.'/vendor/guzzle/guzzle/src/');
JLoader::registerNamespace('Symfony', JPATH_LIBRARIES.'/vendor/symfony/event-dispatcher/');

use Aws\S3\S3Client;
use Aws\S3\Exception\S3Exception;

$s3 = S3Client::factory();
try {
$s3->putObject(array(
'Bucket' => 'my-bucket',
'Key' => 'my-object',
'ACL' => 'public-read',
));
} catch (S3Exception $e) {
echo $e->getMessage();
}

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3617.

avatar haydenyoung haydenyoung - test_item - 17 Oct 2014 - Tested successfully
avatar Bakual Bakual - change - 19 Oct 2014
Labels Added: ?
avatar Bakual
Bakual - comment - 19 Oct 2014

Based on the various tests I'm marking this RTC. Thanks all! This finally clears the way for Joomla! 3.4 Beta :+1:

avatar mbabker mbabker - reference | 9818b27 - 20 Oct 14
avatar b2z
b2z - comment - 20 Oct 2014

WoW! Thanks a lot to all who was working hard on it! :+1:

avatar Bakual
Bakual - comment - 20 Oct 2014

Tried to merge using a diff but had to revert since it messed up the file moving. Will try again later on :)

avatar Bakual Bakual - reference | 4c3df42 - 20 Oct 14
avatar Bakual Bakual - reference | 4e21448 - 20 Oct 14
avatar Bakual Bakual - merge - 20 Oct 2014
avatar Bakual Bakual - close - 20 Oct 2014
avatar Bakual Bakual - close - 20 Oct 2014
avatar Bakual Bakual - change - 20 Oct 2014
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2014-10-20 10:25:27
avatar Bakual
Bakual - comment - 20 Oct 2014

Now merged for real :smile:

Add a Comment

Login with GitHub to post a comment