Fix: Attempt to read property "jsWidget" on null #6941

ArchBlood · 2024-04-09T22:52:52Z

What kind of change does this PR introduce? (check at least one)
fixes #6940

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

It's submitted to the develop branch, not the master branch if no hotfix
When resolving a specific issue, it's referenced in the PR's description (e.g. Fix #xxx[,#xxx], where "xxx" is the Github issue number)
All tests are passing
New/updated tests are included
Changelog was modified

If adding a new feature, the PR's description includes:

A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

yurabakhtin · 2024-04-11T06:49:35Z

@ArchBlood Please check my reviews, but before apply the changes do you know what model has an old wallEntryClass so it calls the deprecated method StreamEntryWidget::renderLegacyWallEntry() and some other inside?
It would be better to find such old model and update it to new version instead of a modifying of the deprecated methods:

StreamEntryWidget::renderLegacyWallEntry()
ContentActiveRecord ::getWallEntryWidget()
ContentActiveRecord::getWallOut()

protected/humhub/modules/content/widgets/stream/StreamEntryWidget.php

ArchBlood · 2024-04-11T08:11:30Z

To answer the main question about which model, it seems it was due to my Ticket model using implements \humhub\modules\content\interfaces\Searchable and using the following still;

    public function getSearchAttributes()
    {
        // Define the searchable attributes
        $attributes = [
            'id' => $this->id,
            'title' => $this->title,
            'description' => $this->description,
        ];

        return $attributes;
    }

yurabakhtin · 2024-04-11T08:44:50Z

@ArchBlood As I understand you have a model like this:

class Ticket extends ContentActiveRecord implements Searchable
{
    ...
    public function getSearchAttributes()
    {
        // Define the searchable attributes
        $attributes = [
            'id' => $this->id,
            'title' => $this->title,
            'description' => $this->description,
        ];

        return $attributes;
    }
    ...
}

But what does the class have for the properties? :

    /**
     * @see StreamEntryWidget
     * @var string the StreamEntryWidget widget class
     */
    public $wallEntryClass;

    /**
     * The stream channel where this content should displayed.
     * Set to null when this content should not appear on streams.
     *
     * @since 1.2
     * @var string|null the stream channel
     */
    protected $streamChannel = 'default';

ArchBlood · 2024-04-11T15:57:11Z

@ArchBlood As I understand you have a model like this:

class Ticket extends ContentActiveRecord implements Searchable
{
    ...
    public function getSearchAttributes()
    {
        // Define the searchable attributes
        $attributes = [
            'id' => $this->id,
            'title' => $this->title,
            'description' => $this->description,
        ];

        return $attributes;
    }
    ...
}

But what does the class have for the properties? :

    /**
     * @see StreamEntryWidget
     * @var string the StreamEntryWidget widget class
     */
    public $wallEntryClass;

    /**
     * The stream channel where this content should displayed.
     * Set to null when this content should not appear on streams.
     *
     * @since 1.2
     * @var string|null the stream channel
     */
    protected $streamChannel = 'default';

Instead of explaining it I've sent you an invite for better visibility of the code.

yurabakhtin · 2024-04-12T09:46:52Z

@ArchBlood

Instead of explaining it I've sent you an invite for better visibility of the code.

Thank you for the access, what I have investigated:

You have the class as searchable class Ticket extends ContentActiveRecord implements Searchable,
and search attibutes are defined in the method getSearchAttributes(), it is ok,
but it strange why the class has no $wallEntryClass,
if I test a model SomeRecord extends ContentActiveRecord implements Searchable without the property $wallEntryClass on previous version 1.15 I get such picture:

I.e. the record is found but it is not rendered, yes we don't have any errors there, but it breaks at least the searching counter.

I advise you either implement the $wallEntryClass for the Ticket or add protected $streamChannel = null; so it excludes the ticket records from wall stream and it will not be visible on the searching page.

But please note the applying $streamChannel = null will works only for new created records because a value is stored in the database column content:stream_channel, so I don't recommend this way.
You can do this only if you really don't need to see the tickets on a wall stream, in such case for proper work you should update the column content:stream_channel to null by migration or manually if it is only on your own DB.

To avoid errors I suggest to update the method StreamEntryWidget::renderLegacyWallEntry like this:

private static function renderLegacyWallEntry(ContentActiveRecord $record, $options = [])
{
    if (!is_array($options)) {
        $options = [];
    }

    if (isset($options['jsWidget'])) {
        $jsWidget = $options['jsWidget'];
        unset($options['jsWidget']);
    } else {
        $wallEntryWidget = $record->getWallEntryWidget();
        $jsWidget = $wallEntryWidget instanceof StreamEntryWidget ? $wallEntryWidget->jsWidget : null;
    }

    if ($jsWidget === null) {
        Yii::error('Model ' . get_class($record) . ' must define $wallEntryClass or set $streamChannel to null!', 'content');
        return '';
    }

    $params = [
        'content' => $record instanceof ContentActiveRecord ? $record->getWallOut($options) : '',
        'jsWidget' => $jsWidget,
        'entry' => $record->content,
    ];

    return Yii::$app->controller->renderPartial('@humhub/modules/content/views/layouts/wallEntry', $params);
}

Could you please test how it works on your DB with already created Tickets?
If all ok I prefer to use this version for your current PR.
Thanks.

ArchBlood · 2024-04-12T12:05:07Z

@ArchBlood

Instead of explaining it I've sent you an invite for better visibility of the code.

Thank you for the access, what I have investigated:

You have the class as searchable class Ticket extends ContentActiveRecord implements Searchable,

and search attibutes are defined in the method getSearchAttributes(), it is ok,

but it strange why the class has no $wallEntryClass,

if I test a model SomeRecord extends ContentActiveRecord implements Searchable without the property $wallEntryClass on previous version 1.15 I get such picture:

I.e. the record is found but it is not rendered, yes we don't have any errors there, but it breaks at least the searching counter.

I advise you either implement the $wallEntryClass for the Ticket or add protected $streamChannel = null; so it excludes the ticket records from wall stream and it will not be visible on the searching page.

But please note the applying $streamChannel = null will works only for new created records because a value is stored in the database column content:stream_channel, so I don't recommend this way. You can do this only if you really don't need to see the tickets on a wall stream, in such case for proper work you should update the column content:stream_channel to null by migration or manually if it is only on your own DB.

To avoid errors I suggest to update the method StreamEntryWidget::renderLegacyWallEntry like this:
private static function renderLegacyWallEntry(ContentActiveRecord $record, $options = [])
{
    if (!is_array($options)) {
        $options = [];
    }

    if (isset($options['jsWidget'])) {
        $jsWidget = $options['jsWidget'];
        unset($options['jsWidget']);
    } else {
        $wallEntryWidget = $record->getWallEntryWidget();
        $jsWidget = $wallEntryWidget instanceof StreamEntryWidget ? $wallEntryWidget->jsWidget : null;
    }

    if ($jsWidget === null) {
        Yii::error('Model ' . get_class($record) . ' must define $wallEntryClass or set $streamChannel to null!', 'content');
        return '';
    }

    $params = [
        'content' => $record instanceof ContentActiveRecord ? $record->getWallOut($options) : '',
        'jsWidget' => $jsWidget,
        'entry' => $record->content,
    ];

    return Yii::$app->controller->renderPartial('@humhub/modules/content/views/layouts/wallEntry', $params);
}
Could you please test how it works on your DB with already created Tickets? If all ok I prefer to use this version for your current PR. Thanks.

I've tested your version with both public $wallEntryClass & protected $streamChannel = null; , where both throw an error as your code intends to do even if they are defined in Ticket model, not really what I was trying to achieve here as an error is still an error if not handled. I've also tried using the following modified version of your snippet with the same issue, so in turn this does not work;

private static function renderLegacyWallEntry(ContentActiveRecord $record, $options = [])
{
    $jsWidget = $options['jsWidget'] ?? $record->getWallEntryWidget()->jsWidget ?? null;

    if ($jsWidget === null) {
        Yii::error('Model ' . get_class($record) . ' must define $wallEntryClass or set $streamChannel to null!', 'content');
        return '';
    }

    $content = $record instanceof ContentActiveRecord ? $record->getWallOut($options) : '';

    $params = [
        'content' => $content,
        'jsWidget' => $jsWidget,
        'entry' => $record->content,
    ];

    return Yii::$app->controller->renderPartial('@humhub/modules/content/views/layouts/wallEntry', $params);
}

yurabakhtin · 2024-04-12T12:31:45Z

private static function renderLegacyWallEntry(ContentActiveRecord $record, $options = [])
{
    $jsWidget = $options['jsWidget'] ?? $record->getWallEntryWidget()->jsWidget ?? null;

    if ($jsWidget === null) {
        Yii::error('Model ' . get_class($record) . ' must define $wallEntryClass or set $streamChannel to null!', 'content');
        return '';
    }

    $content = $record instanceof ContentActiveRecord ? $record->getWallOut($options) : '';

    $params = [
        'content' => $content,
        'jsWidget' => $jsWidget,
        'entry' => $record->content,
    ];

    return Yii::$app->controller->renderPartial('@humhub/modules/content/views/layouts/wallEntry', $params);
}

I guess you have an error when you try to call $record->getWallEntryWidget()->jsWidget but $record->getWallEntryWidget() === null.

In my code I did this:

$wallEntryWidget = $record->getWallEntryWidget();
$jsWidget = $wallEntryWidget instanceof StreamEntryWidget ? $wallEntryWidget->jsWidget : null;

Could you show me please what error did you get when you use my code?

I've tested your version with both public $wallEntryClass

Did you add it like public $wallEntryClass = WallEntry::class;?
Did you create new class WallEntry for the Ticket, as it is implemented for other modules, for example: https://github.com/humhub/news/blob/main/widgets/WallEntry.php
Also you should create a view file like this https://github.com/humhub/news/blob/main/widgets/views/entry.php.

ArchBlood · 2024-04-12T13:19:50Z

private static function renderLegacyWallEntry(ContentActiveRecord $record, $options = [])
{
    $jsWidget = $options['jsWidget'] ?? $record->getWallEntryWidget()->jsWidget ?? null;

    if ($jsWidget === null) {
        Yii::error('Model ' . get_class($record) . ' must define $wallEntryClass or set $streamChannel to null!', 'content');
        return '';
    }

    $content = $record instanceof ContentActiveRecord ? $record->getWallOut($options) : '';

    $params = [
        'content' => $content,
        'jsWidget' => $jsWidget,
        'entry' => $record->content,
    ];

    return Yii::$app->controller->renderPartial('@humhub/modules/content/views/layouts/wallEntry', $params);
}
I guess you have an error when you try to call $record->getWallEntryWidget()->jsWidget but $record->getWallEntryWidget() === null.

In my code I did this:
$wallEntryWidget = $record->getWallEntryWidget();
$jsWidget = $wallEntryWidget instanceof StreamEntryWidget ? $wallEntryWidget->jsWidget : null;
Could you show me please what error did you get when you use my code?

I've tested your version with both public $wallEntryClass

Did you add it like public $wallEntryClass = WallEntry::class;? Did you create new class WallEntry for the Ticket, as it is implemented for other modules, for example: https://github.com/humhub/news/blob/main/widgets/WallEntry.php Also you should create a view file like this https://github.com/humhub/news/blob/main/widgets/views/entry.php.

I'm thrown the following error;

Model gm\humhub\modules\faq\models\Ticket must define $wallEntryClass or set $streamChannel to null!

I've done as the error asks but it shows the error regardless whereas in my p/r code I am not getting any errors at all. 🤔

yurabakhtin · 2024-04-12T16:01:28Z

@ArchBlood I have tested by installing the FAQ module and create a ticket:

Then I try to search it:

Probably we should fix the core class humhub\modules\content\search\SearchRequest to avoid such error for case when some model like Ticket returns null by the method ContentActiveRecord::getContentName().
Please note usually such methods returns some static text, so I suggest to update the method of the Ticket to:

public function getContentName()
{
    return $this->title ?? Yii::t('FaqModule.base', 'Ticket');
}

because it seems the core SearchRequest uses a code like (new Ticket())->getContentName(), so the property Ticket->title is null there.

ok, if I update the method as I suggested above I catch the error Attempt to read property "jsWidget" on null:

Then I update the method StreamEntryWidget::renderLegacyWallEntry() to my suggested code:

private static function renderLegacyWallEntry(ContentActiveRecord $record, $options = [])
{
    if (!is_array($options)) {
        $options = [];
    }

    if (isset($options['jsWidget'])) {
        $jsWidget = $options['jsWidget'];
        unset($options['jsWidget']);
    } else {
        $wallEntryWidget = $record->getWallEntryWidget();
        $jsWidget = $wallEntryWidget instanceof StreamEntryWidget ? $wallEntryWidget->jsWidget : null;
    }

    if ($jsWidget === null) {
        Yii::error('Model ' . get_class($record) . ' must define $wallEntryClass or set $streamChannel to null!', 'content');
        return '';
    }

    $params = [
        'content' => $record instanceof ContentActiveRecord ? $record->getWallOut($options) : '',
        'jsWidget' => $jsWidget,
        'entry' => $record->content,
    ];

    return Yii::$app->controller->renderPartial('@humhub/modules/content/views/layouts/wallEntry', $params);
}

and I can run the search page without errors, only the Ticket record is not visible because WallEntry is not implemented for it yet.

If you need to display the Ticket on the search list you should update the Ticket class by adding the line:

public $wallEntryClass = WallEntry::class;

Then you should add the class WallEntry with code like this:

<?php
namespace gm\humhub\modules\faq\widgets;

use gm\humhub\modules\faq\models\Ticket;
use humhub\modules\content\widgets\stream\WallStreamEntryOptions;
use humhub\modules\content\widgets\stream\WallStreamEntryWidget;
use humhub\modules\content\widgets\stream\WallStreamModuleEntryWidget;

class WallEntry extends WallStreamModuleEntryWidget
{
    /**
     * @inheritdoc
     */
    public $createRoute = '/faq/ticket/create';

    /**
     * @inheritdoc
     * @var Ticket
     */
    public $model;

    /**
     * @inheritdoc
     */
    public $editRoute = '/faq/ticket/update';

    /**
     * @inheritdoc
     */
    public $editMode = WallStreamEntryWidget::EDIT_MODE_NEW_WINDOW;

    /**
     * @inheritdoc
     */
    public function renderContent()
    {
        return $this->render('wall-entry', [
            'ticket' => $this->model,
            'isDetailView' => $this->renderOptions->isViewContext(WallStreamEntryOptions::VIEW_CONTEXT_DETAIL),
        ]);
    }

    /**
     * @inheritdoc
     */
    protected function getTitle()
    {
        return $this->model->title;
    }
}

and also a view file /widgets/views/wall-entry.php:

<?php
use gm\humhub\modules\faq\models\Ticket;
use humhub\modules\content\widgets\richtext\RichText;

/* @var $ticket Ticket */
/* @var $isDetailView bool */
?>

<div data-news="<?= $ticket->id ?>" data-content-key="<?= $ticket->content->id ?>">
    <div data-ui-markdown<?= $isDetailView ? '' : ' data-ui-show-more' ?>>
        <?= RichText::output($ticket->description) ?>
    </div>
</div>

Then a Tciket record will be visbile on the search page like this:

I see the wall entry of the Ticket(as you can see on my screenshot above) even without the modifications in the core method, but it would be better to keep such fixes for some unexpected errors in the broken model like the Ticket, so I hope you update this PR to that my suggested code and I will approve this PR, otherwise please show me why it doesn't work for you, thanks.

But if you don't need to search ticket records then you can just remove implements Searchable and the method public function getSearchAttributes().

Please let me know if have some questions else, thanks.

ArchBlood · 2024-04-12T16:24:40Z

But if you don't need to search ticket records then you can just remove implements Searchable and the method public function getSearchAttributes().

Please let me know if have some questions else, thanks.

The reason why the ticket was made searchable is so that it was easier for the user to find, of course we didn't want it to return the actual ticket information through a WallEntry::class due to privacy concerns;

Going back to using protected $streamChannel = null; this does the same as using public $wallEntryClass = WallEntry::class;

What @GreenMeteor wants to do is make use of the search function without the use of displaying the actual information in the search stream which both of the above does with the added error applied.

This P/R fixes the original reported error and makes it possible to use the search function how we needed it without using either $wallEntryClass or $streamChannel

what-the-diff · 2024-04-12T20:29:18Z

PR Summary

Improved Null-checks and Error Logging
This change enhances the robustness of our code. If certain objects ($wallEntryWidget, $jsWidget) in StreamEntryWidget.php are null, we'll now check for that and act appropriately instead of the code simply crashing. This makes the system less likely to experience unforeseen errors.
Enhanced Handling for Non-ContentActiveRecord Instances
Certain functions in the software were originally built to operate only on instances of ContentActiveRecord. We've modified the code to handle cases where non-ContentActiveRecord instances might be introduced. It now uses suitable fallback values, which ensures consistent functioning of the system in these situations.
Safe Assignments for Variables
If $wallEntryWidget is an instance of StreamEntryWidget, the $jsWidget is now assigned to use this value. Also, the $content is now updated to use $record->getWallOut($options) only if $record is a ContentActiveRecord. These adjustments ensure the variables are assigned meaningful and appropriate values reducing the chances of errors.

ArchBlood · 2024-04-12T20:30:11Z

I'll have to go with your suggestion and implement some restrictions @yurabakhtin so that only the creator can view their own Tickets.

yurabakhtin · 2024-04-15T06:37:19Z

@ArchBlood I see you updated this PR as I recommended, thanks, I can approve it now, so it works like this without changes in the Ticket class:

It may be a confused in the counter part, because Ticket records are not visible.

But as I understand you don't want to implement $wallEntryClass fo the Ticket class because it will be visible on the Dashboard:

@luke- Currently we index content records only with $streamChannel = 'default', the code is here: $this->content->stream_channel === Content::STREAM_CHANNEL_DEFAULT.

What do you think if we will introduce new stream_channel = 'search' and index such content records as well?

So then @ArchBlood could make the Tickets are visbile only on the search list and hidden on Dashboard stream:

class Ticket extends ContentActiveRecord implements Searchable
{
    public $wallEntryClass = WallEntry::class;

    public $streamChannel = Content::STREAM_CHANNEL_SEARCH; // 'search'

luke- · 2024-04-15T21:52:53Z

@yurabakhtin If the content should not appear on the dashboard, can't we just use the new Visbility flag?

Otherwise, we also have modules that simply implement a separate stream channel in such cases: https://github.com/humhub/translation/blob/master/models/TranslationLog.php#L41

Do we need to change anything else in the PR in this regard?

protected/humhub/modules/content/widgets/stream/StreamEntryWidget.php

luke- · 2024-04-22T14:30:27Z

@yurabakhtin Why do we need the new checks for instanceof StreamEntryWidget and ContentActiveRecord?

yurabakhtin · 2024-04-22T15:25:51Z

Why do we need the new checks for instanceof StreamEntryWidget and ContentActiveRecord?

@luke- I thought we should avoid a displaying of the error, but if you prefer don't touch core code and fix the external model with wrong properties instead, then yes we can skip this PR.

luke- · 2024-04-22T15:37:41Z

@yurabakhtin Ok the error is fine but why we need 'content' => $record instanceof ContentActiveRecord ? $record->getWallOut($options) : '',?

yurabakhtin · 2024-04-22T16:45:28Z

@yurabakhtin Ok the error is fine but why we need 'content' => $record instanceof ContentActiveRecord ? $record->getWallOut($options) : '',?

@luke- I confirm, we don't need this change at all because I don't know how such case may happens, I already answered about this here, so the change can be reverted.

luke- · 2024-04-23T06:33:50Z

Ok, thanks for the explanation:

From my side, we can either:

keep the PR and revert the following line: https://github.com/humhub/humhub/pull/6941/files#diff-708348c91e9bdcfac2cf80d59dbff60f3b13e95b8da5bdeb13cedc3783eec5bdR119 - Then we would have an error message if StreamEntry is not defined
close the PR and fix the module page if necessary

ArchBlood · 2024-05-06T00:27:34Z

Ok, thanks for the explanation:

From my side, we can either:

keep the PR and revert the following line: https://github.com/humhub/humhub/pull/6941/files#diff-708348c91e9bdcfac2cf80d59dbff60f3b13e95b8da5bdeb13cedc3783eec5bdR119 - Then we would have an error message if StreamEntry is not defined

close the PR and fix the module page if necessary

I've reverted it back the line mentioned incase of this P/R being accepted.

Fix: Attempt to read property "jsWidget" on null

f97d73f

luke- requested a review from yurabakhtin April 10, 2024 07:08

luke- assigned yurabakhtin Apr 10, 2024

yurabakhtin requested changes Apr 11, 2024

View reviewed changes

Update StreamEntryWidget.php

044e1e0

ArchBlood requested a review from yurabakhtin April 12, 2024 20:29

yurabakhtin approved these changes Apr 15, 2024

View reviewed changes

luke- requested changes Apr 15, 2024

View reviewed changes

protected/humhub/modules/content/widgets/stream/StreamEntryWidget.php Outdated Show resolved Hide resolved

protected/humhub/modules/content/widgets/stream/StreamEntryWidget.php Show resolved Hide resolved

Update StreamEntryWidget.php

1e23c52

luke- approved these changes May 6, 2024

View reviewed changes

luke- added this pull request to the merge queue May 6, 2024

Merged via the queue into humhub:develop with commit c62c187 May 6, 2024
6 checks passed

ArchBlood deleted the patch-1 branch May 6, 2024 15:35

ArchBlood mentioned this pull request May 17, 2024

User::getSearchAttributes() removed in HumHub 1.16 #7008

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Attempt to read property "jsWidget" on null #6941

Fix: Attempt to read property "jsWidget" on null #6941

ArchBlood commented Apr 9, 2024 •

edited

yurabakhtin commented Apr 11, 2024

ArchBlood commented Apr 11, 2024

yurabakhtin commented Apr 11, 2024 •

edited

ArchBlood commented Apr 11, 2024

yurabakhtin commented Apr 12, 2024

ArchBlood commented Apr 12, 2024

yurabakhtin commented Apr 12, 2024

ArchBlood commented Apr 12, 2024

yurabakhtin commented Apr 12, 2024

ArchBlood commented Apr 12, 2024

what-the-diff bot commented Apr 12, 2024

ArchBlood commented Apr 12, 2024

yurabakhtin commented Apr 15, 2024

luke- commented Apr 15, 2024

luke- commented Apr 22, 2024

yurabakhtin commented Apr 22, 2024

luke- commented Apr 22, 2024

yurabakhtin commented Apr 22, 2024

luke- commented Apr 23, 2024

ArchBlood commented May 6, 2024

Fix: Attempt to read property "jsWidget" on null #6941

Fix: Attempt to read property "jsWidget" on null #6941

Conversation

ArchBlood commented Apr 9, 2024 • edited

yurabakhtin commented Apr 11, 2024

ArchBlood commented Apr 11, 2024

yurabakhtin commented Apr 11, 2024 • edited

ArchBlood commented Apr 11, 2024

yurabakhtin commented Apr 12, 2024

ArchBlood commented Apr 12, 2024

yurabakhtin commented Apr 12, 2024

ArchBlood commented Apr 12, 2024

yurabakhtin commented Apr 12, 2024

ArchBlood commented Apr 12, 2024

what-the-diff bot commented Apr 12, 2024

PR Summary

ArchBlood commented Apr 12, 2024

yurabakhtin commented Apr 15, 2024

luke- commented Apr 15, 2024

luke- commented Apr 22, 2024

yurabakhtin commented Apr 22, 2024

luke- commented Apr 22, 2024

yurabakhtin commented Apr 22, 2024

luke- commented Apr 23, 2024

ArchBlood commented May 6, 2024

ArchBlood commented Apr 9, 2024 •

edited

yurabakhtin commented Apr 11, 2024 •

edited