@bembelimen

• nice feature for sites that want to offer backend access and put limits on menu usage

the testing instructions are incomplete,

• you include new column 'asset_id' for TABLE menu_types, but you do not warn to do "DB-fix"

NEW Sites: patch works properly
EXISTING Sites: the site login and assets break badly as soon as you save a menu type

• tested 2 times on existing installations of staging with no other patches appied, after saving the menu type ([EDIT] NOTE: change at least 1 permission before saving) login to backend was possible only for super admins, all other usergroups (e.g. administrator) can not login (despite having both backend login, note just before clicking save, the login for them was possible)

It worked for new installation,

• applied patch before running Joomla installation, run the installation, and the patch worked, the ACL permissions were saved per menu-type and respected for the given user-group

There are changes into the installation folder into the SQL files, you are modifying the inital rows of the assets TABLE.

• i guess that is ok ?

but you need to see how to handle upgrading existing sites too

Hello @ggppdk,
thank you for testing.
Could you please tell me,

• how you did the update
• what was changed in the #__assets tables?

Confirming that a database fix is required after applying the patch - please add to the test intructions

I tested this on an new 3.5.2 dev successfully. I've inspected the asset table and the rules were set properly for all com_menu.menu.x records.
Additionally I installed the patch via patchtester on a existing 3.5.1. - successfully here too.
It was necessary to do the db-fix. I cannot confirm the effect of @ggppdk.
There must be a coincidence - but I have no idea.

• a small mistake on my part, you do need to CHANGE at least 1 ACL permission so that a new asset gets created
• and of course you need to do the above by editing a menu type and not a menu item

Furthermore i found why login to all user fails after saving (except for super admins)

• it is becase the newly created asset has parent_id zero (and also has level 0)

e.g. the following asset is after denying create on the "administrator" usergroup for "about Joomla" menu type

• the created asset is like this:

INSERT INTO
jrv3e_assets (id, parent_id, lft, rgt, level, name, title, rules) VALUES
(181, 0, 438, 439, 0, 'com_menus.menu.4', 'About Joomla', '{"core.create":{"7":0}}');

The reason is because _getAssetParentId() is not called and this is because the "asset_id" column in menu_types table was not created,

• it is not created because Joomla schema check does not catch the case of "ADD" it only catches "ADD COLUMN" (i was supposed to make a PR for this, but did not do at the time i reported it)

so it is simple to fix ... change *.sql files used for updating existing sites ...

Hello @ggppdk YES! Thank you for this find!
I updated the code.

I have tested this item successfully on 1a96f6c

Cool feature, Tab appears, Permission settings working also.

I have tested this item successfully on 1a96f6c

@test successful 3.5.2 dev and 3.5.1

Just want to say the comments blocks are not correct (@since 3.5, and some don't have the since), when this probably will go for 3.6 since it add new methods.

It already has two tests, so i guess a maintainer can correct that when merging.

It would be better if the doc blocks were updated by the creator of this PR

This PR has received new commits.

CC: @chmst, @designbengel

Ok, I updated the "since" parameter to 3.6

@wilsonge were the @since changes in /libraries/legacy/table/menu/type.php correct?

If so then this can be RTC as it had two successful tests before the docblock changes

@bembelimen now we have merge conflicts can you have a look? If it is back in sync you can ping me and i set it back to RTC. Thanks.

This PR has received new commits.

CC: @chmst, @designbengel

Thanks -> RTC

@zero-24 Thanks, the problem was, that the "Show all items" feature was looks very "cobbled together". I had to clean up some stuff to implement the ACL check again.

So all conflics should be solved now.

Merged - thankyou!

It looks like this has totally broken #10190

????

In which way?

No more All menu items here

@infograf768 Thanks, readded the entry: #10279

Also, batch has changed I had a special batch body to prevent using batch when we have the All menu items on

Please create a new issue and dont comment on closed issues

On 7 May 2016 at 16:36, infograf768 notifications@github.com wrote:

Also, batch has changed I had a special batch body to prevent using batch
when we have the All menu items on

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9814 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

What a surprise it was @wilsonge that broke the web

Come on
This was merged without checking stuff

@bembelimen

This was the bactchcopy content

<?php
/**
 * @package     Joomla.Administrator
 * @subpackage  com_menus
 *
 * @copyright   Copyright (C) 2005 - 2016 Open Source Matters, Inc. All rights reserved.
 * @license     GNU General Public License version 2 or later; see LICENSE.txt
 */
defined('_JEXEC') or die;

$options = array(
    JHtml::_('select.option', 'c', JText::_('JLIB_HTML_BATCH_COPY')),
    JHtml::_('select.option', 'm', JText::_('JLIB_HTML_BATCH_MOVE'))
);
$published = $this->state->get('filter.published');
$menuType = (array) JFactory::getApplication()->getUserState('com_menus.items.menutype');
?>
<?php if (!empty($menuType)) : ?>
    <div class="row-fluid">
        <div class="control-group span6">
            <div class="controls">
                <?php echo JHtml::_('batch.language'); ?>
            </div>
        </div>
        <div class="control-group span6">
            <div class="controls">
                <?php echo JHtml::_('batch.access'); ?>
            </div>
        </div>
    </div>
    <div class="row-fluid">
        <?php if ($published >= 0) : ?>
            <div id="batch-choose-action" class="combo control-group">
                <label id="batch-choose-action-lbl" class="control-label" for="batch-choose-action">
                    <?php echo JText::_('COM_MENUS_BATCH_MENU_LABEL'); ?>
                </label>
                <div class="controls">
                    <select name="batch[menu_id]" id="batch-menu-id">
                        <option value=""><?php echo JText::_('JLIB_HTML_BATCH_NO_CATEGORY') ?></option>
                        <?php echo JHtml::_('select.options', JHtml::_('menu.menuitems', array('published' => $published))); ?>
                    </select>
                </div>
            </div>
            <div id="batch-copy-move" class="control-group radio">
                <?php echo JText::_('JLIB_HTML_BATCH_MOVE_QUESTION'); ?>
                <?php echo JHtml::_('select.radiolist', $options, 'batch[move_copy]', '', 'value', 'text', 'm'); ?>
            </div>
        <?php endif; ?>
    </div>
<?php else : ?>
    <div class="row-fluid">
        <p><?php echo JText::_('COM_MENUS_SELECT_MENU_FIRST') ?></p>
    </div>
<?php endif; ?>

Also, batch has changed I had a special batch body to prevent using batch when we have the All menu items on

That made no sense, why prevent batch? The batch process is independed from the choosen menutype (still works for "all")

Nono
See #8411 (comment)
It is very important, as you can easily mistake when choosing stuff from different menus

Because one can't know for sure what will happen when choosing menu items from different menus and move or copy them to another menu that may be a menu to which belongs one of the menu items selected.
Another aspect would be the various parameters of these selected menu items (language, access) that could/would have to be normalised in that case and, if not, may also create confusion.
Another problem is also the hierarchy of the items selected.

Please just reinstate #10190 as it was merged

see: #10281

I don't like the idea to deacticate functionality just because the new feature (= show all items) did not implement this function properly, but OK, I added the check again...