Coding standards

silvi.addweb → made their first commit to this issue’s fork.

Hello,

I created an MR and fixed phpcs problems. Review, please.

The mentioned page issue is resolved, & I have also checked and it is working as expected. I am attaching the Screenshots & doing RTBC for the same.

I'm getting an error with the Merge Request branch, on php 8.3.

$ curl https://views-ical.ddev.site/events.ics
The website encountered an unexpected error. Try again later.<br><br><em class="placeholder">ArgumentCountError</em>: Too few arguments to function Drupal\views_ical\ViewsIcalHelper::__construct(), 0 passed in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 261 and exactly 2 expected in <em class="placeholder">Drupal\views_ical\ViewsIcalHelper-&gt;__construct()</em> (line <em class="placeholder">47</em> of <em class="placeholder">modules/contrib/views_ical/src/ViewsIcalHelper.php</em>). <pre class="backtrace">Drupal\Component\DependencyInjection\Container-&gt;createService() (Line: 179)
Drupal\Component\DependencyInjection\Container-&gt;get() (Line: 92)
Drupal\views_ical\Plugin\views\style\IcalWizard::create() (Line: 21)
Drupal\Core\Plugin\Factory\ContainerFactory-&gt;createInstance() (Line: 83)
Drupal\Component\Plugin\PluginManagerBase-&gt;createInstance() (Line: 824)
Drupal\views\Plugin\views\display\DisplayPluginBase-&gt;getPlugin() (Line: 941)
Drupal\views\ViewExecutable-&gt;initStyle() (Line: 921)
Drupal\views\ViewExecutable-&gt;getStyle() (Line: 482)
Drupal\views\Plugin\views\field\FieldPluginBase-&gt;defineOptions() (Line: 407)
Drupal\views\Plugin\views\field\EntityField-&gt;defineOptions() (Line: 143)
Drupal\views\Plugin\views\PluginBase-&gt;init() (Line: 110)
Drupal\views\Plugin\views\HandlerBase-&gt;init() (Line: 142)
Drupal\views\Plugin\views\field\FieldPluginBase-&gt;init() (Line: 218)
Drupal\views\Plugin\views\field\EntityField-&gt;init() (Line: 902)
Drupal\views\Plugin\views\display\DisplayPluginBase-&gt;getHandlers() (Line: 1099)
Drupal\views\ViewExecutable-&gt;_initHandler() (Line: 957)
Drupal\views\ViewExecutable-&gt;initHandlers() (Line: 2326)
Drupal\views\Plugin\views\display\DisplayPluginBase-&gt;preExecute() (Line: 1751)
Drupal\views\ViewExecutable-&gt;preExecute() (Line: 1686)
Drupal\views\ViewExecutable-&gt;executeDisplay() (Line: 81)
Drupal\views\Element\View::preRenderViewElement()
call_user_func_array() (Line: 113)
Drupal\Core\Render\Renderer-&gt;doTrustedCallback() (Line: 870)
Drupal\Core\Render\Renderer-&gt;doCallback() (Line: 432)
Drupal\Core\Render\Renderer-&gt;doRender() (Line: 248)
Drupal\Core\Render\Renderer-&gt;render() (Line: 153)
Drupal\Core\Render\Renderer-&gt;Drupal\Core\Render\{closure}() (Line: 638)
Drupal\Core\Render\Renderer-&gt;executeInRenderContext() (Line: 152)
Drupal\Core\Render\Renderer-&gt;renderRoot() (Line: 110)
Drupal\views\Plugin\views\display\Feed::buildResponse() (Line: 140)
Drupal\views_ical\Plugin\views\display\IcalDisplay::buildResponse() (Line: 56)
Drupal\views\Routing\ViewPageController-&gt;handle()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 638)
Drupal\Core\Render\Renderer-&gt;executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel-&gt;handle() (Line: 53)
Drupal\Core\StackMiddleware\Session-&gt;handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength-&gt;handle() (Line: 191)
Drupal\page_cache\StackMiddleware\PageCache-&gt;fetch() (Line: 128)
Drupal\page_cache\StackMiddleware\PageCache-&gt;lookup() (Line: 82)
Drupal\page_cache\StackMiddleware\PageCache-&gt;handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState-&gt;handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel-&gt;handle() (Line: 741)
Drupal\Core\DrupalKernel-&gt;handle() (Line: 19)

We weren't really using ViewsIcalHelper as a proper service before. This patch does that, but we weren't defining any arguments for ViewsIcalHelper in views_ical.services.yml. Adding:

arguments: ['@date.formatter', '@token.entity_mapper']

Sort of broke on me, because we have an implicit assumption that the token module is present and installed, but the views_ical module itself doesn't declare token as a dependency. I'm not really sure how to manage a soft dependency like that. While I'm hesitant to add new dependencies in general, token has over half a million installs, so this one might be alright. I had a question of what to do about the lock file update, since I can't really run composer commands to drupal module dependencies within the module, and it looks like the general practice is not to ship contrib modules with lock files. So I'll just remove that.

Looking more at the patch, It's also adding another dependency to Smart Date via this "SmartDateTrait". Which we should only be optionally supporting if the site already has Smart Date installed, has configured an entity to use Smart Date fields, and then configured the ical view to use those fields. It shouldn't be declared as a trait to one of our classes like this, and I'm not really sure why it's even needed at all here.

As an aside, I get the impression that this patch was put together in an effort to farm contribution credit, and that the author(s) didn't have a local environment running, or make any attempt to test the patch in such an environment. If I see such posts in my module queue again, I will mark them as spam. If you use this module in your own projects, and are genuinely interested in contributing to it, then I am grateful for any effort there, but blindly posting untested patches just adds more work for maintainers to validate.

Automated testing is enabled in 📌 Write tests Active .

Testing is now enabled.

Rebased; phpcs now passes.

Thank you for your work on this. I'll need to do some testing though. I don't remember if $this->themeFunctions() was doing something in the background that we needed. We obviously didn't need it in a render array, if we weren't returning it. It's been a bit since I've dived deep into this code, and I've had an aspiration to just tear this whole thing down and rebuild it from scratch for a while now.

It seems likely that $rows doesn't have to be generated either. That was put out of use in #3084714: Render with Views API more properly. → , commit b9e2350.