Improvement - Githubissues

Hi @cdaecke,

I have tested this extension and have some improvement ideas:

jQuery

Better idea: Just migrate your scripts form jQuery to vanilla JS. I think only your script uses it, fullcalendarv5 does not need it. There are even tools for that :wink:

Installation

~~Add a note to the installation instruction that jQuery is required.~~

Check if missing

~~Additionally, I sugges t to add a check in your template and display an message, if jQuery is missing. This could look like this (add to Show.html):~~

if ('undefined' == typeof window.jQuery) {
    calendarEl<f:format.raw>{contentObject.uid}</f:format.raw>.innerHTML = "Missing jQuery.";
    return;
}

Configuration

In the installation instruction you say that the configuration (e.g. defaultDetailPid) should be set via modifying the setup.typoscript inside your extension. If I understood it right I think this is not a good practice. I suggest, to introduce more constants and reference them similar to {$plugin.tx_mdfullcalendar_cal.persistence.storagePid}.

This way the configuration can be done via typoscript without modifying file and more flexibility.

Query

storagePid

The storage pid is a parameter in the AJAX request (&storage=27,29,52). This can be modified by an enduser to any PID. This way they could gain access to calendarize entries on any storage page. In my opinion this is a security weakness.

What are the reason why this even URL parameter?

My remidation suggestion is to remove this parameter, or if not possible limit it to the pages of plugin/typoscript configuration.

Start/end date

These parameters are also freely choosable by the end user. A malicious user could set the timerange e.g. from 2000 to 2030. On a server with many indices this could put a big load on it or even be used as a Denial of Service attack.

My suggestions:

limit the timespan between start and end date to e.g. to a bit more than one month
maybe also introduce a limit for the minimum and maximum date (similar to settings.dateLimitBrowserPrev in calendarize) via typoscript

Ajax

Why does the ajaxUrl (1573760945) point to defaultDetailPid? I think it should point to the same page as your plugin, not the detail page with the calendarize plugin. My suggestion is to rust remove the setTargetPageType for the ajax url here: https://github.com/cdaecke/md_fullcalendar/blob/ebdfe88a1407d5692a257ec088943df8cdd2c7d3/Classes/Controller/CalController.php#L146-L156

Other

JSON response

Instead of echoing the json response, you could better use return $this->jsonResponse(json_encode($items)); (I think available since Typo3 v11)

https://github.com/cdaecke/md_fullcalendar/blob/ebdfe88a1407d5692a257ec088943df8cdd2c7d3/Classes/Controller/CalController.php#L192-L194

Links

The links in locationLink and organizerLink could also be typolinks (e.g. t3://page?uid=51) and part of the JSON response. Are these links used anywhere?

If you want to resolve them you could use the TYPO3\CMS\Core\LinkHandling\LinkService with ->resolve(...) in your controller. https://github.com/cdaecke/md_fullcalendar/blob/ebdfe88a1407d5692a257ec088943df8cdd2c7d3/Classes/Controller/CalController.php#L181-L185

Modal

When you have Bootstrap installed the modal is used. When the ajax request to the backend fails, the mouse cursor keeps spinning/waiting. Additionally, the user does not notice the error.

Similar to the success, you can add a error function (modify Show.html)

$.ajax({
    url: item.event.extendedProps.uriAjax,
    success: function(result){
        calModal.find('.modal-content-inner').html(result);
        calModal.modal('show');
    },
    error: function(result) {
        // Notify the user, e.g. by a bootstrap toast or an alert box
        alert("Error occured.please try again");
    },
    complete: function() {
        // Always remove the cursor, on success and on failure
        $(document.body).css({'cursor' : ''});
    },
});

I know this a LOT. I hope this still helps you and feel free to reach out to me.

Hi @okmiim ,

thanks a lot for your improvement ideas, they are appreciated very much!

Now, as ext:calendarize is available for TYPO3 v12, I have turned my attention back to extension.

Please find my feedback below:

jQuery

Better idea: Just migrate your scripts form jQuery to vanilla JS. I think only your script uses it, fullcalendarv5 does not need it. There are even tools for that 😉

Would you like to make a proposal (pull request)?

~Installation~

~Add a note to the installation instruction that jQuery is required.~

~Check if missing~

~Additionally, I sugges t to add a check in your template and display an message, if jQuery is missing. This could look like this (add to Show.html):~
if ('undefined' == typeof window.jQuery) {
    calendarEl<f:format.raw>{contentObject.uid}</f:format.raw>.innerHTML = "Missing jQuery.";
    return;
}

Is added, thanks!

Configuration

In the installation instruction you say that the configuration (e.g. defaultDetailPid) should be set via modifying the setup.typoscript inside your extension. If I understood it right I think this is not a good practice. I suggest, to introduce more constants and reference them similar to {$plugin.tx_mdfullcalendar_cal.persistence.storagePid}.

This way the configuration can be done via typoscript without modifying file and more flexibility.

Is updated.

Query

storagePid

The storage pid is a parameter in the AJAX request (&storage=27,29,52). This can be modified by an enduser to any PID. This way they could gain access to calendarize entries on any storage page. In my opinion this is a security weakness.

What are the reason why this even URL parameter?

This was introduced for reasons of flexibility. This makes it possible for a TYPO3 editor to configure the storages of calendaize records.

Would you just allow to configure it by the constant {$plugin.tx_mdfullcalendar_cal.persistence.storagePid}?

Start/end date

These parameters are also freely choosable by the end user. A malicious user could set the timerange e.g. from 2000 to 2030. On a server with many indices this could put a big load on it or even be used as a Denial of Service attack.

My suggestions:
* limit the timespan between start and end date to e.g. to a bit more than one month

* maybe also introduce a limit for the minimum and maximum date (similar to `settings.dateLimitBrowserPrev` in calendarize) via typoscript

Timespan check is added. https://github.com/cdaecke/md_fullcalendar/blob/8d1605d27f00a3bfb5dcb01a56b433b071f4255b/Classes/Controller/CalController.php#L115-L120

Ajax

Why does the ajaxUrl (1573760945) point to defaultDetailPid? I think it should point to the same page as your plugin, not the detail page with the calendarize plugin.

The link to the detail page is added as fallback, so the link to the "real" calendarize entry is in place. Do you see any problems with that?

Other

JSON response

Instead of echoing the json response, you could better use return $this->jsonResponse(json_encode($items)); (I think available since Typo3 v11)

This is changed, thanks for the input!

Links

The links in locationLink and organizerLink could also be typolinks (e.g. t3://page?uid=51) and part of the JSON response. Are these links used anywhere?

The links are used in the Detail.html template and will be processed by the fluid viewhelper <f:link.typolink parameter="{event.organizerLink}">{event.organizer}</f:link.typolink> https://github.com/cdaecke/md_fullcalendar/blob/8d1605d27f00a3bfb5dcb01a56b433b071f4255b/Resources/Private/Templates/Cal/Detail.html#L75-L82

If you want to resolve them you could use the TYPO3\CMS\Core\LinkHandling\LinkService with ->resolve(...) in your controller.

https://github.com/cdaecke/md_fullcalendar/blob/ebdfe88a1407d5692a257ec088943df8cdd2c7d3/Classes/Controller/CalController.php#L181-L185

Modal

When you have Bootstrap installed the modal is used. When the ajax request to the backend fails, the mouse cursor keeps spinning/waiting. Additionally, the user does not notice the error.

Changes are added.

I know this a LOT. I hope this still helps you and feel free to reach out to me.

Thanks again your input!

If you have more improvments, let me know, or add changes by opening a pull request.

Please find all changes in the attached commit.

cdaecke / md_fullcalendar

Improvement #26

jQuery

Installation

Check if missing

Configuration

Query

storagePid

Start/end date

Ajax

Other

JSON response

Links

Modal

jQuery

~Installation~

~Check if missing~

Configuration

Query

storagePid

Start/end date

Ajax

Other

JSON response

Links

Modal