Skip to content

[3.9] modify installer to allow additional query params#21309

Merged
mbabker merged 3 commits intojoomla:3.9-devfrom
clayfreeman:3.9-dev
Aug 2, 2018
Merged

[3.9] modify installer to allow additional query params#21309
mbabker merged 3 commits intojoomla:3.9-devfrom
clayfreeman:3.9-dev

Conversation

@clayfreeman
Copy link
Copy Markdown
Contributor

@clayfreeman clayfreeman commented Jul 30, 2018

Is your feature request related to a problem? Please describe.

Components are unable to specify additional query parameters when defining primary admin menu items in the manifest file. Submenu items, however, are able to specify additional query parameters.

Describe the solution you'd like

Patch this file to allow additional query parameters as specified in the component manifest, similar to how it is done for submenu items. See the changes in this pull request to apply the patch and test it.

Additional context

It should be possible to build components without the use of a "generic" controller that defines a default view. Menu items should be able to specify the task and view query parameters so that there is no ambiguity as to which controller and view class should be used.

Example

task and view can now be set in the primary menu item so that the default administration controller doesn't have to be used for the primary menu item.

<administration>
  ...
  <menu task='test.display' view='test'>Test</menu>
  ...
</administration>

For a component com_blah, the above code example will create a menu item that points to index.php?option=com_blah&task=test.display&view=test. This means that BlahControllerTest, BlahModelTest and BlahViewTest will be used to handle the request as opposed to the generic, component-wide BlahController and its BlahView$default_view class.

`Joomla\CMS\Installer\Adapter\ComponentAdapter` should allow (at a minimum) the
same subset of query parameters for primary admin menu items that is allowed for
submenu items.

If desired, this can be expanded to allow full `link` override in the manifest
file like currently possible for submenu items.
@wilsonge
Copy link
Copy Markdown
Contributor

I can see why you would want to allow task/view as you described in your issue (although don't feel strongly about it). However neither act nor sub are things used in core therefore I definitely am against them being magically whitelisted when they have no definition.

@clayfreeman
Copy link
Copy Markdown
Contributor Author

@wilsonge I've stripped down the list of allowed parameters. Please see my revision.

@clayfreeman
Copy link
Copy Markdown
Contributor Author

I'm not quite sure why the AppVeyor build failed; logically there was no difference from that merge commit.

Regardless, is there an update on this?

@ghost
Copy link
Copy Markdown

ghost commented Aug 1, 2018

@clayfreeman next this PR needs two sucessfully Tests.

@clayfreeman
Copy link
Copy Markdown
Contributor Author

clayfreeman commented Aug 1, 2018 via email

@ghost
Copy link
Copy Markdown

ghost commented Aug 1, 2018

Everyone can test (Test by PR-Creator doesn't count as its expected that he test before create Pull Request).

Edit: Its helpful to give Instructions for Tester in the PR, a Link to an Issue without Instructions isn't helpful to know what to test.

@joomla-cms-bot joomla-cms-bot changed the title modify installer to allow additional query params [3.9] modify installer to allow additional query params Aug 1, 2018
@ghost
Copy link
Copy Markdown

ghost commented Aug 1, 2018

Changed Title to show its about 3.9.


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

@mbabker
Copy link
Copy Markdown
Contributor

mbabker commented Aug 1, 2018

By Joomla developers or community members?

I have been running this patch for days with no issue.

All patches must be tested/reviewed by at least two people other than the person who submitted the pull request.

@travisrisner
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on adf71c7.

@jeremyvii
Copy link
Copy Markdown

I have also tested this item ✅ successfully on adf71c7.

@ghost
Copy link
Copy Markdown

ghost commented Aug 1, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added RTC This Pull Request is Ready To Commit and removed RTC This Pull Request is Ready To Commit labels Aug 1, 2018
@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Aug 2, 2018

Thankyou for your first contribution to the CMS!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants