GSOC - media package for the platform #1759

Closed
wants to merge 4 commits into
from

Projects

None yet

3 participants

@kaviththiranga
Media package manual

https://github.com/kaviththiranga/joomla-platform/blob/media_package/docs/manual/en-US/chapters/packages/media.md

Previous discussion thread

https://groups.google.com/forum/?fromgroups=#!topic/joomla-dev-platform/gsozoo4B2Jw

In the previous discussion, some modification were suggested such as removing the usage of JFile, refactoring JMediaCombiner as JMediaCollection etc.

I am creating this pull request, after committing those modifications.

Media package

https://github.com/kaviththiranga/joomla-platform/tree/media_package/libraries/joomla/media

Tests for media package

https://github.com/kaviththiranga/joomla-platform/tree/media_package/tests/suites/unit/joomla/media

@kaviththiranga

Unit tests are running fine locally. But one test fails when tests are run via PullTester. The below assertion

https://github.com/kaviththiranga/joomla-platform/blob/media_package/tests/suites/unit/joomla/media/compressor/JMediaCompressorJsTest.php#L97

compresses contents of a file and checks it with the contents of the file named filename.min.js. Seems like it compresses the contents of case1.js and compares it with the contents of jquery-1.2.3.min.js, instead of comparing with case1.min.js.

But it runs fine locally. Am I doing something wrong with the file paths?

Below is the folder of javascript test files
https://github.com/kaviththiranga/joomla-platform/tree/media_package/tests/media/js

@elinw
Contributor
elinw commented Dec 23, 2012

I'm not sure but when this happens to me (working locally not on jenkins) it usually means that I need to use reflection for something that I didn't use it for.

@ianmacl
Contributor
ianmacl commented Dec 25, 2012

So I'll let others weigh in here, but this isn't how I would build the interface for this.

Some might have noticed by now that I'm not a huge fan of statics. Historically we used them all over the place and they have caused us a lot of trouble. I don't see a great need for them here.

I would sooner see something like JMediaCollection be instantiated with plain old new.

i.e. $js = new JMediaCollectionJs($options);

I think you want to allow for more flexibility in terms of available compressors as there seem to be variety of schemes used and this is something that is changing. I would probable make JMediaCompressor an interface with two abstract objects implementing it - JMediaCompressorJs and JMediaCompressorCss. Then you would have one or more concrete classes that extend these two abstract base classes - these classes are where the actual work of compression would happen. They would be something like:
JMediaCompressorJsYui
JMediaCompressorJsNone
JMediaCompressorJsUglify
...

JMediaCompressorCssYui
JMediaCompressorCssNone
...

You probably want to have some sort of signature method for JMediaCollection so that you don't have to generate a file every time - you only want to generate the file when the file doesn't already exist.

New classes should conform to the standard in use by the autoloader and so you don't really need to have an isSupported method - the one you have limits compressors to the current directory which is going to limit people who want to use their own. Instead people can just use class_exists or at the least you could make a simple isSupported method that does the class exists for people.

I would add a setCompressor method to the JMediaCollection class and combine the two using composition. In this way, it is very easy to change the compression method used by the collection object. Then you have a toFile method or something in JMediaCollection that will write the collection to a file.

You have made a good start and I commend your effort in putting this out there. I think we just need a few tweaks to get it ready to add to the platform.

@kaviththiranga

Hi, thanks for your review. Actually in the original plan JMediaCompressor was an interface. But later it was changed. The idea of further extending comnpressors like JMediaCompressorJsYui seems great. So do you suggest that I should remove static methods?
Honestly, a goal of creating this pull request was to get some effective feedback on design from maintainers. Actually the originally suggested design was changed few times and I have modified the library few times according to them.

I do not have a big experience in these kind of things. But with some guidance, I think I will be able to make this great.
I am really happy to work on this further, so that I can make my summer work useful to others. If you all can give me some last decisions on design, I am ready to work on.

@elinw
Contributor
elinw commented Mar 18, 2013

Kavith any updates?

@kaviththiranga

Hi Elin,

Need few more hours work. I will make the pull request from new branch ASAP.

On Tue, Mar 19, 2013 at 1:55 AM, elinw notifications@github.com wrote:

Kavith any updates?


Reply to this email directly or view it on GitHubhttps://github.com/joomla/joomla-platform/pull/1759#issuecomment-15079332
.

Kavith Thiranga Lokuhewage,
Undergraduate,
BSc (Hons) Software Engineering,
Staffordshire University, UK.

@kaviththiranga

Hi all,
I am closing this pull request and I will make another pull request from the new branch as soon as I am done with modifications.
Thanks.

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