Skip to content

WR-483012 Fix PHP 8.2 deprecation for dynamic property $rows#224

Merged
guillogo merged 1 commit into
catalyst:MOODLE_500_STABLEfrom
agaranin:MOODLE_500_STABLE_wr483012
Mar 10, 2026
Merged

WR-483012 Fix PHP 8.2 deprecation for dynamic property $rows#224
guillogo merged 1 commit into
catalyst:MOODLE_500_STABLEfrom
agaranin:MOODLE_500_STABLE_wr483012

Conversation

@agaranin
Copy link
Copy Markdown

@agaranin agaranin commented Mar 9, 2026

No description provided.

Comment thread classes/admin_setting_sql_textarea.php Outdated
*/
class admin_setting_sql_textarea extends \admin_setting_configtextarea {
/** @var int Number of rows for the textarea. */
public $rows;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@agaranin Can you check whether line 45 is actually doing anything? As $this->rows does not update the parent's private $rows property, so it may have no effect on the rendered textarea. When calling $html = parent::output_html($data, $query); it will use parent rows.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch, Guillermo! You were right — line 45 had no effect because $this->rows was writing to the child class's own property, not the parent's private $rows which parent::output_html() actually reads. The textarea height was always controlled by the JS resize() function below, not by the PHP $rows value. I've removed the dead code (public $rows, MIN_ROWS constant, and the assignment on line 45) and force-pushed the fix.

@agaranin agaranin force-pushed the MOODLE_500_STABLE_wr483012 branch from 3bb7f6e to 67ba1f4 Compare March 10, 2026 19:39
@guillogo guillogo merged commit c465365 into catalyst:MOODLE_500_STABLE Mar 10, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants