User tests: Successful: Unsuccessful:
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:
libraries/vendor
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)JApplicationBase
now extends Joomla\Application\AbstractApplication
and now duplicated code or class variables in that class and child objects are cleaned upTODO:
I looked at it. Typehinting, protected methods being renamed and declared private; can't happen.
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
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.
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?
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.
I try my level headed best not to take accountability for anything :D
I rest my case, Your Honor. :-D
Travis fails, but other than that, can we separate the phpmailer update from this to make testing easier.
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?
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
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?
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
@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.
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
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.
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.
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
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).
i would recommend to introduce a new directory-constant
I feel that that we do not need more constants.
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.
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.
Is IIS still not case sensitive ? I thought they wanted to change that.
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
Okay, never mind. I still dont like libraries/vendor ;) Maybe we will find a better solution in the future.
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).
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.
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.).
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.).
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
My "vision" would be to have a www folder or similar, containing the "web root".
Remember it when we will discuss Joomla 4
;)
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
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.
Any update on this?
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.
Labels |
Removed:
?
|
The preference is libraries/vendor
and that's what the current PR code is supporting.
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.
Status | New | ⇒ | Pending |
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.
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?
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.
Plus it still doesn't actually fix the unit tests :P
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
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.
Unit tests are now failing after the JString changes :P
Source of unit test failures: joomla-framework/string@8b47188#commitcomment-7833874
Milestone |
Added: |
Milestone |
Added: |
Updated test packages are posted to http://developer.joomla.org/PR-packages/3617/ in sync with commit b6031f9
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.
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.
@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.
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?
@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
@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
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.
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.
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.
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.
@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.
Updated test packages are posted to http://developer.joomla.org/PR-packages/3617/ in sync with commit 26f09fa
Labels |
Added:
?
|
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.
Labels |
Added:
?
|
Based on the various tests I'm marking this RTC. Thanks all! This finally clears the way for Joomla! 3.4 Beta
WoW! Thanks a lot to all who was working hard on it!
Tried to merge using a diff but had to revert since it messed up the file moving. Will try again later on :)
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-10-20 10:25:27 |
Now merged for real
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.