Skip to content

Conversation

harshkhatri8
Copy link

@harshkhatri8 harshkhatri8 commented Oct 6, 2025

Pull Request for Issue #45603
Fixes #45603

Screenshot 2025-10-06 221905

Summary of Changes

-Updated ClientModel.php to prevent duplicate client names when using Save as Copy.
-Implemented automatic unique name generation: if a client with the same name exists, the system appends (2), (3), etc.
-Preserves the original Save as Copy functionality without breaking existing save operations.

Testing Instructions

-Go to Components → Banners → Clients in Joomla Administrator.
-Select an existing client,.
-Click Save as Copy multiple times.
-Verify that each new client is created with a unique name:
-Confirm that no duplicates are created in the database and all other save functionality works as expected.

Actual result BEFORE applying this Pull Request

-Clicking Save as Copy creates a client with the same name as the original, allowing duplicates.

Expected result AFTER applying this Pull Request

-Clicking Save as Copy automatically generates a unique client name, preventing duplicates while preserving the copy functionality.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

… duplicate record in Component -> Banners -> Clients
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-5.3-dev labels Oct 6, 2025
@brianteeman
Copy link
Contributor

This is obviously not correct. With this pr you can never Save as copy so why keep the button. Please look at how Save as copy works elsewhere!

@harshkhatri8 harshkhatri8 reopened this Oct 6, 2025
@harshkhatri8
Copy link
Author

This is obviously not correct. With this pr you can never Save as copy so why keep the button. Please look at how Save as copy works elsewhere!

Thanks for the feedback, @brianteeman. I see your point now — my current PR prevents Save as Copy from working as intended. I’ll update the PR so that:

Save / Save & Close still prevent duplicates.

Save as Copy will create a copy automatically, appending a suffix like it works if a client with the same name exists.

This should fix the original issue while keeping Joomla’s copy functionality intact. I’ll push an updated version soon.

… duplicate record in Component -> Banners -> Clients
@harshkhatri8
Copy link
Author

harshkhatri8 commented Oct 6, 2025

This is obviously not correct. With this pr you can never Save as copy so why keep the button. Please look at how Save as copy works elsewhere!

i have made all the necessary changes can you please now review it.
Screenshot 2025-10-06 221905

Comment on lines 140 to 141
/**
* 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.

*
* @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__

Comment on lines 165 to 175



/**
* 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)
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)

@richard67
Copy link
Member

@harshkhatri8 When you create a PR you should check how your changes look on GitHub: https://github.com/joomla/joomla-cms/pull/46239/files . Then you would see it has a bunch of code style issues.

Comment on lines 188 to 192
}




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.

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
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.

Comment on lines 14 to 17
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.

Comment on lines 164 to 174
}



/**
* Generate a unique client name if it already exists.
*
* @param string $name The original client name
*
* @return string Unique client name
* @since __DEPLOY_VERSION__
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
}
/**
* Generate a unique client name if it already exists.
*
* @param string $name The original client name
*
* @return string Unique client name
* @since __DEPLOY_VERSION__
}
/**
* Generate a unique client name if it already exists.
*
* @param string $name The original client name
*
* @return string Unique client name
* @since __DEPLOY_VERSION__

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.

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.

@HLeithner HLeithner changed the base branch from 5.3-dev to 5.4-dev October 15, 2025 17:08
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.4-dev.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Banner clients accept duplicates

6 participants