Skip to content

Update insert-data example to fix errors #466

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

cyrez
Copy link
Contributor

@cyrez cyrez commented May 14, 2025

User description

If we run this code example, it will return errors:

// Bind values
$query
    ->bind(':user_id', 1001, Joomla\Database\ParameterType::INTEGER)
    ->bind(':profile_key', 'custom.message')
    ->bind(':profile_value', 'Inserting a record using insert()')
    ->bind(':ordering', 1, Joomla\Database\ParameterType::INTEGER);

Bind issue with integer

Since we are not handing a variable but an integer, it generates Fatal error: Cannot pass parameter 2 by reference

Instead of:

$query
    ->bind(':user_id', 1001, Joomla\Database\ParameterType::INTEGER)

better:

$user_id = 1001;

$query
    ->bind(':user_id', $user_id, \Joomla\Database\ParameterType::INTEGER)

Namespace issue

And ParameterType is not found as not backslashed.

Should be (proposed solution):

$query
    ->bind(':user_id', $user_id, \Joomla\Database\ParameterType::INTEGER)

or:

use Joomla\Database\ParameterType;

[...]

$query
    ->bind(':user_id', $user_id, ParameterType::INTEGER)

Other places in the manual can also be adjusted.


PR Type

Documentation


Description

  • Fixes incorrect parameter binding in code example.

  • Adds quoting for integer values in bind statements.

  • Ensures correct namespacing for ParameterType usage.


Changes walkthrough 📝

Relevant files
Documentation
insert-data.md
Fix parameter binding and namespace in database insert example

versioned_docs/version-5.3/general-concepts/database/insert-data.md

  • Updates bind statements to use quoted integer values.
  • Adds leading backslash to ParameterType namespace.
  • Improves code example accuracy for parameter binding.
  • +2/-2     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Incorrect Solution

    The PR uses $db->quote() inside bind() which is unnecessary and potentially problematic. The bind() method already handles parameter quoting internally. The correct solution should use variables instead of literals as mentioned in the PR description.

    ->bind(':user_id', $db->quote(1001), \Joomla\Database\ParameterType::INTEGER)
    ->bind(':profile_key', 'custom.message')
    ->bind(':profile_value', 'Inserting a record using insert()')
    ->bind(':ordering', $db->quote(1), \Joomla\Database\ParameterType::INTEGER);

    Copy link
    Contributor

    qodo-merge-pro bot commented May 14, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Remove redundant value quoting
    Suggestion Impact:The commit removed the redundant $db->quote() calls as suggested, but also went further by extracting the values into variables before binding them

    code diff:

    -    ->bind(':user_id', $db->quote(1001), \Joomla\Database\ParameterType::INTEGER)
    -    ->bind(':profile_key', 'custom.message')
    -    ->bind(':profile_value', 'Inserting a record using insert()')
    -    ->bind(':ordering', $db->quote(1), \Joomla\Database\ParameterType::INTEGER);
    +    ->bind(':user_id', $user_id, \Joomla\Database\ParameterType::INTEGER)
    +    ->bind(':profile_key', $profile_key)
    +    ->bind(':profile_value', $profile_value)
    +    ->bind(':ordering', $ordering, \Joomla\Database\ParameterType::INTEGER);

    Remove the $db->quote() calls when using the bind() method with parameter types.
    The bind() method already handles proper escaping based on the parameter type,
    and double-quoting will cause incorrect SQL to be generated.

    versioned_docs/version-5.3/general-concepts/database/insert-data.md [70-74]

     $query
    -    ->bind(':user_id', $db->quote(1001), \Joomla\Database\ParameterType::INTEGER)
    +    ->bind(':user_id', 1001, \Joomla\Database\ParameterType::INTEGER)
         ->bind(':profile_key', 'custom.message')
         ->bind(':profile_value', 'Inserting a record using insert()')
    -    ->bind(':ordering', $db->quote(1), \Joomla\Database\ParameterType::INTEGER);
    +    ->bind(':ordering', 1, \Joomla\Database\ParameterType::INTEGER);

    [Suggestion processed]

    Suggestion importance[1-10]: 9

    __

    Why: This suggestion is highly relevant and correct: using $db->quote() within bind() when specifying a parameter type is redundant and can cause SQL errors due to double quoting. Removing the quoting improves correctness and prevents potential bugs in SQL statement execution.

    High
    • Update

    @HLeithner
    Copy link
    Member

    the qodo bot mentioned already the incorrect solution, please correct your change.

    @cyrez
    Copy link
    Contributor Author

    cyrez commented May 14, 2025

    the qodo bot mentioned already the incorrect solution, please correct your change.

    If i change for ->bind(':user_id', 1001, \Joomla\Database\ParameterType::INTEGER) i've got this error:

    0 - Joomla\Database\DatabaseQuery::bind(): Argument #2 ($value) cannot be passed by reference

    So, better to set variable before to bind pass?

    @HLeithner
    Copy link
    Member

    Yes you need to set a variable because the value is a reference, normally that doesn't matter because you don't use bind for constants and instead write it directly into the query but for documentation you need a variable.

    @ceford
    Copy link
    Collaborator

    ceford commented May 15, 2025

    You also need to set variables for profile_key and profile_value and bind them as STRING:

    $query->bind(':profileKey', $profileKey, ParameterType::STRING);
    Perhaps just explain that the variables are defined/passed before using the database call.

    @cyrez
    Copy link
    Contributor Author

    cyrez commented May 15, 2025

    @HLeithner @ceford i've updated the PR by adding variables.

    Could it be ok like this? What can be improved?

    Thanks!

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

    Successfully merging this pull request may close these issues.

    3 participants