Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions administrator/components/com_banners/src/Model/ClientModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Joomla\CMS\MVC\Model\AdminModel;
use Joomla\CMS\Table\Table;
use Joomla\CMS\Versioning\VersionableModelTrait;
use Joomla\CMS\Language\Text;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use Joomla\CMS\MVC\Model\AdminModel;
use Joomla\CMS\Table\Table;
use Joomla\CMS\Versioning\VersionableModelTrait;
use Joomla\CMS\Language\Text;
use Joomla\CMS\Language\Text;
use Joomla\CMS\MVC\Model\AdminModel;
use Joomla\CMS\Table\Table;
use Joomla\CMS\Versioning\VersionableModelTrait;

Sort alpha order.


// phpcs:disable PSR1.Files.SideEffects
\defined('_JEXEC') or die;
Expand Down Expand Up @@ -135,4 +136,58 @@ protected function prepareTable($table)
{
$table->name = htmlspecialchars_decode($table->name, ENT_QUOTES);
}

/**
* Override save to prevent duplicate client names when saving as copy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Override save to prevent duplicate client names when saving as copy.
/**
* Override save to prevent duplicate client names when saving as copy.

*
* @param array $data The form data.
*
* @return boolean True on success, false on failure.
*
* @since 5.3.5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @since 5.3.5
* @since __DEPLOY_VERSION__

*/
public function save($data)
{
$table = $this->getTable();

// If saving as copy or duplicate exists, generate a unique name
if (isset($data['name'])) {
$table->load(['name' => $data['name']]);

// Only modify name if it's a duplicate
if ($table->id) {
$data['name'] = $this->generateUniqueName($data['name']);
}
}

return parent::save($data);
}



/**
* Generate a unique client name if it already exists.
*
* @param string $name The original client name
*
* @return string Unique client name
*/
protected function generateUniqueName($name)
Comment on lines 165 to 175
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Generate a unique client name if it already exists.
*
* @param string $name The original client name
*
* @return string Unique client name
*/
protected function generateUniqueName($name)
/**
* Generate a unique client name if it already exists.
*
* @param string $name The original client name
*
* @return string Unique client name
* @since __DEPLOY_VERSION__
*/
protected function generateUniqueName($name)

{
$table = $this->getTable();
$baseName = $name;
$i = 2;

// Keep appending numbers until the name is unique
while ($table->load(['name' => $name])) {
$name = $baseName . ' (' . $i . ')';
$i++;
}

return $name;
}




Copy link
Member

@richard67 richard67 Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the 4 empty lines 189 to 192.

}
2 changes: 2 additions & 0 deletions administrator/language/en-GB/com_banners.ini
Original file line number Diff line number Diff line change
Expand Up @@ -202,5 +202,7 @@ COM_BANNERS_TYPE2="Clicks"
COM_BANNERS_UNLIMITED="Unlimited"
COM_BANNERS_WARNING_PROVIDE_VALID_NAME="Please provide a valid, non-blank name"
COM_BANNERS_XML_DESCRIPTION="This component manages banners and banner clients."
COM_BANNERS_CLIENT_ERROR_DUPLICATE_NAME="A client with this name already exists."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move language key in alpha order.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is also done now you can check it.


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Remove redundant empty line.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, @richard67!
I’ve applied all your suggested code style changes — fixed the docblocks, removed extra blank lines, and added @SInCE DEPLOY_VERSION.
The PR has been updated and is ready for another review. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Language key should be in alpha order.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @QuyTon!
I made all the suggested changes. Please check it and provide a feedback.


JLIB_RULES_SETTING_NOTES_COM_BANNERS="Changes apply to this component only.<br><em><strong>Inherited</strong></em> - a Global Configuration setting or higher level setting is applied.<br><em><strong>Denied</strong></em> always wins - whatever is set at the Global or higher level and applies to all child elements.<br><em><strong>Allowed</strong></em> will enable the action for this component unless overruled by a Global Configuration setting." ; Alternate language strings for the rules form field
Loading