This repository has been archived by the owner. It is now read-only.

[fix] findmanifest should prioritize main folder manifests #1842

Merged
merged 1 commit into from Mar 18, 2013

Conversation

Projects
None yet
4 participants
@phproberto
Contributor

phproberto commented Mar 4, 2013

Actually the findManifest function looks for manifests in the parent folder and in first level subfolders.

This example structure:

->fof
--->fof.xml
-libraries
-manifies.xml

Will return the fof.xml manifiest.

This fix ensures that the function prioritises the main folder manifiests. I would like to remove the subfolder check but kept it for B/C reasons.

@dongilbert

This comment has been minimized.

Show comment
Hide comment
@dongilbert

dongilbert Mar 4, 2013

Contributor

Since this is a CMS specific issue, I would defer the check on this to someone like @dextercowley or @elinw to make sure it is the desired functionality.

Contributor

dongilbert commented Mar 4, 2013

Since this is a CMS specific issue, I would defer the check on this to someone like @dextercowley or @elinw to make sure it is the desired functionality.

@phproberto

This comment has been minimized.

Show comment
Hide comment
@phproberto

phproberto Mar 4, 2013

Contributor

I see it as a Platform bug. I think "anything" using JInstaller expects it to load first the main manifest instead of subfolder's.

I already tested it and sent PR for the 2.5 and the 3.0 CMS branches:

3.0:
joomla/joomla-cms#752

2.5:
joomla/joomla-cms#753

Thanks for reviewing this!

Contributor

phproberto commented Mar 4, 2013

I see it as a Platform bug. I think "anything" using JInstaller expects it to load first the main manifest instead of subfolder's.

I already tested it and sent PR for the 2.5 and the 3.0 CMS branches:

3.0:
joomla/joomla-cms#752

2.5:
joomla/joomla-cms#753

Thanks for reviewing this!

@elinw

This comment has been minimized.

Show comment
Hide comment
@elinw

elinw Mar 4, 2013

Contributor

Is there any idea that any application that isn't the CMS is using JInstaller? The code seems so specific that I think we might want to move the whole package from legacy to cms so that it's easier to make it work correctly for the cms.

Contributor

elinw commented Mar 4, 2013

Is there any idea that any application that isn't the CMS is using JInstaller? The code seems so specific that I think we might want to move the whole package from legacy to cms so that it's easier to make it work correctly for the cms.

@dongilbert

This comment has been minimized.

Show comment
Hide comment
@dongilbert

dongilbert Mar 4, 2013

Contributor

I would agree with that Elin, which is the reason for my first comment. It's not that I don't want to fix or I don't think it's a bug, it's just highly CMS specific, IMO.

Contributor

dongilbert commented Mar 4, 2013

I would agree with that Elin, which is the reason for my first comment. It's not that I don't want to fix or I don't think it's a bug, it's just highly CMS specific, IMO.

@AmyStephen

This comment has been minimized.

Show comment
Hide comment
@AmyStephen

AmyStephen Mar 4, 2013

Contributor

@elinw probably not, the paths are so mixed in, I wasn't able to use it.

Contributor

AmyStephen commented Mar 4, 2013

@elinw probably not, the paths are so mixed in, I wasn't able to use it.

@dongilbert

This comment has been minimized.

Show comment
Hide comment
@dongilbert

dongilbert Mar 16, 2013

Contributor

So I agree with Elin, JInstaller should move to the CMS. Any opposition?

Contributor

dongilbert commented Mar 16, 2013

So I agree with Elin, JInstaller should move to the CMS. Any opposition?

@AmyStephen

This comment has been minimized.

Show comment
Hide comment
@AmyStephen

AmyStephen Mar 16, 2013

Contributor

Nope. Makes sense to me.

Contributor

AmyStephen commented Mar 16, 2013

Nope. Makes sense to me.

@dongilbert

This comment has been minimized.

Show comment
Hide comment
@dongilbert

dongilbert Mar 18, 2013

Contributor

@elinw Do you want to merge this PR before the package moves to CMS?

Contributor

dongilbert commented Mar 18, 2013

@elinw Do you want to merge this PR before the package moves to CMS?

@elinw

This comment has been minimized.

Show comment
Hide comment
@elinw

elinw Mar 18, 2013

Contributor

I thought we already merged it?

Contributor

elinw commented Mar 18, 2013

I thought we already merged it?

dongilbert added a commit that referenced this pull request Mar 18, 2013

Merge pull request #1842 from phproberto/manifest
[fix] findmanifest should prioritize main folder manifests

@dongilbert dongilbert merged commit 887601e into joomla:staging Mar 18, 2013

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