Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Validation::run() accepts DB connection #8188

Closed
wants to merge 1 commit into from

Conversation

ciadavid
Copy link

@ciadavid ciadavid commented Nov 10, 2023

Description
See #6723

Remove on run() $dbGroup ?string because it can allow group array connections, not only dgGroup Name.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Remove on run() $dbGroup ?string because it can allow group array connections, not only dgGroup Name.
@kenjis kenjis added wrong branch PRs sent to wrong branch breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities docs needed Pull requests needing documentation write-ups and/or revisions. tests needed Pull requests that need tests labels Nov 10, 2023
@kenjis
Copy link
Member

kenjis commented Nov 10, 2023

This is a breaking change.
See https://3v4l.org/VMDWp

@kenjis kenjis changed the title Update Validation.php feat: Validation::run() accepts DB connection Nov 14, 2023
@kenjis
Copy link
Member

kenjis commented Nov 22, 2023

For this PR to be merged, it is necessary to have at least the following:

  • Change the destination of the PR to the next minor version branch (4.5 at the time of writing) instead of develop
  • Add explanations to the changelog and upgrading guide regarding the breaking change
  • Add test code for the added functionality

If you have any question, feel free to ask.

@kenjis kenjis added the waiting for info Issues or pull requests that need further clarification from the author label Nov 22, 2023
@lonnieezell
Copy link
Member

I see nothing wrong with the intention of this PR, and I don't really find it to be a BC break, since it's opening up new capabilities not restricting. As mentioned, we would need tests added and the interface would also need to be updated.

@ciadavid Are you able to tackle this? If not, we'll go ahead and close this.

@ciadavid
Copy link
Author

how can I write a test?

//DB conection array
$conection_array = array(
	'hostname' => 'localhost',
	'username' => 'user',
	'password' => 'pass',
	'database' => 'database',
	'DBDriver' => 'MySQLi',
	'port'     => 3306,	
);

//Default config
//File /Config/Databse.php 'default' the same as $conection_array

//Create table users whit field name unique

$validation_rule = array(
	'Name' => 'required|is_not_unique[users.name,Name]',
);

//Whit interface updated.

$validation = \Config\Services::validation();
$validation->setRules($validation_rule);
$isValid = $validation->run(array('Name' => 'John Doe'),null,$conection_array);

$isValid = $validation->run(array('Name' => ''),null,'default');

//Both are working whitout errors.

//Whit interface not updated.
$isValid = $validation->run(array('Name' => 'John Doe'),null,$conection_array); ///Error third parameter must be an string beacuse is only allower to use the connection string name.


/**
 * Runs the validation process, returning true/false determining whether
	 * validation was successful or not.
	 *
	 * @param array|null  $data    The array of data to validate.
	 * @param string|null $group   The predefined group of rules to apply.
	 * @param string|null $dbGroup The database group to use.
 */
// public function run(?array $data = null, ?string $group = null, $dbGroup = null): bool;

// When is_unique rule is used, it tray co connect to dabase, here is allowed to use the connection string name or an array with the connection data.
/*
	public function is_not_unique(?string $str, string $field, array $data): bool
	{
		// Grab any data for exclusion of a single row.
		[$field, $whereField, $whereValue] = array_pad(
			explode(',', $field),
			3,
			null
		);
		// Break the table and field apart
		sscanf($field, '%[^.].%[^.]', $table, $field);

		$row = Database::connect($data['DBGroup'] ?? null)
			->table($table)
			->select('1')
			->where($field, $str)
			->limit(1);
		if (! empty($whereField) && ! empty($whereValue)
					&& ! preg_match('/^\{(\w+)\}$/', $whereValue)
		) {
			$row = $row->where($whereField, $whereValue);
		}

		return $row->get()->getRow() !== null;
	}

the Database::connect($data['DBGroup'] ?? null) allow to send array whit database connection or the group name.

*/

@kenjis
Copy link
Member

kenjis commented Dec 28, 2023

@lonnieezell The intention of this PR is not wrong.
But see https://3v4l.org/VMDWp
If a dev overrides Validation::run(), it will break, because of Fatal error.
So according our current policy, it is a breaking change.

Therefore we don't accept this change in develop branch.
This must go to 4.5 branch.

@kenjis
Copy link
Member

kenjis commented Dec 28, 2023

@ciadavid Have you ever written test code?
If you are not familiar with testing, see https://github.com/codeigniter4/CodeIgniter4/tree/develop/tests#resources

This is the test class for Database Related Rules Test:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php

@kenjis
Copy link
Member

kenjis commented Dec 28, 2023

@kenjis
Copy link
Member

kenjis commented Feb 1, 2024

I sent another PR #8499

@kenjis
Copy link
Member

kenjis commented Feb 7, 2024

Closed by #8499

@kenjis kenjis closed this Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities docs needed Pull requests needing documentation write-ups and/or revisions. enhancement PRs that improve existing functionalities tests needed Pull requests that need tests waiting for info Issues or pull requests that need further clarification from the author wrong branch PRs sent to wrong branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants