Hi folks, thanks for putting this together and sharing!
@peter pulsifer let me know if this patch fixes the issue for you, thanks!
https://git.drupalcode.org/project/feeds_ical/-/merge_requests/18.diff
https://www.drupal.org/project/feeds_ical/issues/3517541 🐛 Provide timezone fields and retain UNIX TIMESTAMP Active
ben.hamelin → created an issue.
@peter pulsifer Sorry I am battling with Gitlab.... going to open a new issue so this is quicker. I can't seem to rebase the issue fork from the source project from the Gitlab UI.
Thanks @peter pulsifer I will take a look at this tomorrow. That mapping should indeed pass the timestamp directly.
Are you able to share an example .ics / .ical that you're working with?
@peter pulsifer, @devad
Please see open MR and patch and let me know if this works for you.
Thanks!
Noting we decided to include this in 3515185
Recalling this request previously. Seems harmless to include.
Patch from #6 and instructions from #7 worked for me on version: Entity Usage 8.x-2.0-beta17
Patch is simple, looks good.
I'd propose we get this merged in given low risk, noted value from community.
Released in 2.3.2
Thanks @das.gautam and @someshver
Going to release this shortly
Noting that the MR diff does not apply cleanly to 3.0.2.
I had to use the patch from #4.
I could not get either approach to work (MBU form or DropzoneJS) without patch.
I also used "public://tmp" as my upload location (and created "tmp" folder in sites/default/files), and set Form Mode to "none"
@rushikesh-raval
Following up here, I've added a new Feeds Parser plugin config form to the module in this issue: https://www.drupal.org/project/feeds_ical/issues/3323737 ✨ Feeds Ical module doesn't have functionality to import events before/after specific time period. Active
In addition other small fixes over last few weeks.
I'm hoping this new code gets this closer to where it needs to be for coverage. Additionally, as another potential source of reviewable code, I'm wondering if adding some automated unit and functional tests might suffice?
Noting also that from the comments #3 and #4 that I have been the only regular committer over the last few years.
Thank you!
I am working on this but the module is small and that is presenting some hurdles. I am going to see about recent updates and the potential to add automated testing as a way to potentially leap those and get this under coverage. As a fallback I may look to add another maintainer who can opt it in.
@someshver opened merge request for this work. Adds a new parser config form and updates parser logic to exclude any event older than "day before" setting. Leave blank or enter 0 to process entire feed as before.
Hoping this is still useful for folks!
@someshver I think we can handle this with the $filterDaysBefore argument. This will require a new Parser form for config.
I believe this issue will be resolved by the same fix in: https://www.drupal.org/project/feeds_ical/issues/3062885 🐛 Timezone selection not working properly RTBC
To confirm you can try the patch from that MR here: https://git.drupalcode.org/project/feeds_ical/-/merge_requests/13.diff
I tested with your 2 examples and now timezone is handled properly.
Please let me know either here or in that issue if this resolves for you, thanks!
This new MR addresses the timezone issue more directly. Checking for "T" isn't reliable given the various forms of date strings we might see, including something like "DTSTART;TZID=America/New_York:19980312T083000"
The ICal\ICal class provides the timezone in the date arrays like
Array ( [0] => Array ( [TZID] => Europe/Berlin ) [1] => 20240410T140000 [2] => 1712750400 [3] => TZID=Europe/Berlin:20240410T140000 )
In this way, we can identify if a timezone was provided and use it appropriately, which in the case above involves formatting the date string like:
20240410T140000Europe/Berlin
See related Feeds issue: https://www.drupal.org/project/feeds/issues/3070757 →
With this MR, formatting using the provided timezone results in the Feeds DateTargetBase class creating the date using the provided timezone. See: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Datetime%...
"Note that the $timezone parameter and the current
* timezone are ignored when the $time parameter either is a UNIX timestamp
* (e.g. @946684800) or specifies a timezone
* (e.g. 2010-01-28T15:00:00+02:00)."
In my testing imported events without timezone were unaffected, while events with them were adjusted accordingly to match my site's default setting.
If I could get some testing here that would be awesome! Thanks!
@keinstein Did some work on this today, and have a fix that provides LAST-MODIFIED as a timestamp like Drupal expects. However, it's worth noting this Feeds issue: https://www.drupal.org/project/feeds/issues/3060308 ✨ Add a mapping target for "changed" (Updated date) Needs work
Without a presave or postsave hook the default Drupal entity update handler is going to use the current time regardless of what you have in the feed. This fix is still needed to ensure we have a properly formatted timestamp for mapping.
I found this doc when searching for new Drupal Recipe info related to migrations. I wanted to clarify that the doc does not refer at all to the new Drupal Recipes initiative.
Thanks @akafitty and @rcodina! I am going to look into this soon. Are you able to paste a text version of your ICS file, or upload it here? It can be truncated for testing.
Also to confirm, are your timezone settings in the site America/Vancouver?
Additionally how are your date fields configured on your events content type?
Should we close in favor of https://www.drupal.org/project/seckit/issues/3185024 ✨ Change Feature Policy to Permissions Policy (D8/D9) Needs work ?
Closing this as it is stale and for Drupal 7.
Just came across this during setup and same problem. Reviewed the MR it looks good, updated status to get this updated documentation committed. Thanks all!
ben.hamelin → made their first commit to this issue’s fork.
Thank you @rushikesh-raval
I am planning some feature upgrades for the Feeds iCal module that I will share once they are committed. 'm hopeing these enhancements will provide sufficient PHP code to meet these requirements.
Good morning! Just chiming in here, this fixed an issue we were dealing with where the breadcrumb was being driven by the assignment to a Group. Alias was derived from the group path, but breadcrumb was being cached. Thanks everyone!
Any technical solution exclusively in Drupal code is always going to run up against the hosting environment limitations. One way I could see this working would be with a SMTP server provided by the Drupal Association. You would then need an account on d.o. plus DNS configuration to enable sending. Then the service could be supported $$ wise by association funds, donations, etc.
@codekarate or other maintainers. What's the status of this module?
Good morning @avpaderno. I'd like to re-open this request as there have been a number of recent improvements to the Feeds iCal module, in addition to other contributions I've made in the past few years. Here are some of the recent fixes I've directly written code for:
https://www.drupal.org/project/project_browser/issues/3293909
🐛
Security and maintained icons need to communicate correct information
Fixed
https://www.drupal.org/project/feeds_ical/issues/3445551
🐛
PHP 8.2: Creation of dynamic property is deprecated
Fixed
https://www.drupal.org/project/feeds_ical/issues/3379345
🐛
Deprecated function in ICal
Fixed
Additionally, I am co-maintainer of the following modules:
https://www.drupal.org/project/sfds →
https://www.drupal.org/project/layout_section_fields →
Hoping this additional work is sufficient, but if not I have some features I'm planning for Feeds iCal which I think would provide enough additional PHP code to cover the requirements.
Thanks!
- Ben
Release 2.3.0 contains the new fields
@vasyok - Thank you for that clarification and additional info. I understand now that you wish to have access to the raw values for DTSTART and DTEND. I have opened this issue in response to address: https://www.drupal.org/project/feeds_ical/issues/3477371 ✨ Provide the DTSTART and DTEND values as raw strings Active
ben.hamelin → created an issue.
@vasyok Not sure, but it looks like you are mapping to a TEXT field that has a character limit. I would map those to Date / Time fields.
As this appears to be an unrelated issue and the original request has been addressed, I'm going to close this issue.
Thanks for using the module, if you have any suggestions for documentation or examples you want to add please feel free to open a merge request!
That shouldn't be happening. LOCATION is provided as one of the default fields since it is expected in an ical feed.
https://icalendar.org/
Can you provide an example of your ICS / ICAL source file?
@vasyok - The only way to move any of the Predefined source fields outside that group would be to override the src/Feeds/Parser/IcalParser.php class, specifically the getMappingSources() method.
To my knowledge the group they are in has no affect on the final mapping. What is your goal in trying to regroup them?
Hi @VasyOK,
The "Predefined" source values come from `src/Feeds/Parser/IcalParser.php` parser plugin. See https://git.drupalcode.org/project/feeds_ical/-/blob/2.x-dev/src/Feeds/P...
The "Feed Entity" group is provided by Feeds module.
The "Ical" group is added once you start adding custom source values. I did not see the Ical group until I added a test source in my local development environment.
@johnpicozzi Ok updated styles and addressed the linting and FunctionalJavascript tests. This is ready for another look!
I've rebased from 2.0.x and updated the markup to include the button tag with title attribute as suggested. This is ready for review.
ben.hamelin → changed the visibility of the branch 3293909-security-and-maintained to hidden.
@rockmachine Thanks for the follow up. Glad things are working for you now!
To future proof your code changes you should introduce a custom module: https://www.drupal.org/docs/develop/creating-modules →
The goal here would be to create a sub class of the IcalItem class that contains the properties you need to override. This will keep your code intact in your new custom module, and allow the Feeds iCal module to continue to receive updates as they are released. Otherwise you will need to refrain from updating the module which is not best practice.
@artinruins Thanks, does that imply we would need the legend as proposed in:
https://www.drupal.org/project/project_browser/issues/3282163
✨
Improve iconography usability by adding a legend
Needs work
Is there an approach where we use a button without a labeldby attribute, and perhaps place the title attribute on the button tag?
<button class="pb-project__status-icon" title="This module is covered by the Drupal Security Team."><img src="/modules/contrib/project_browser/images/blue-security-shield-icon.svg" class="pb-icon pb-icon--false pb-icon--status false" alt="Security Advisory Coverage" ></button>
@bnjmnm do you have other expectations here, or can anyone point to an existing core UI that does something similar? I thought the "Status report" page might but I don't see title attributes on those icons.
Working on these updates, including adding "title" as noted, as well as "tabindex" to enable tabbing. Is it appropriate to add "tabindex" here, or should we rather add an anchor tag which would support tabbing by default without the need for the tabindex. The links could go to relevant d.o docs.
ben.hamelin → made their first commit to this issue’s fork.
Hi @rockmachine - Just wanted to check in and see how things are working for you. Were any of these suggestions helpful?
@rockmachine Are you able to provide some details on the following?
- How many events are in the ICS feed? How large is the file?
- What is the data structure for one of your events? Can you share an example, as I'm interested to know how many properties are in each event.
- What version of PHP are you running?
- What are the system resources (RAM, CPU)?
- Can you share code snippets of what you changed? How did you make these changes, manually or did you use a custom sub class?
This certainly seems like it could be a result from adding the "additionalFields" properties in https://www.drupal.org/project/feeds_ical/issues/3358713 ✨ Add additional fields Fixed
We did address a similar report in https://www.drupal.org/project/feeds_ical/issues/3445551 🐛 PHP 8.2: Creation of dynamic property is deprecated Fixed
However, I'm also wondering how this might be working if each event has a large number of properties, and if there are many events, if we are simply seeing the performance issues in writing watchdog logs during the import. Assuming a high number of deprecation errors per event across a high volume of events would potentially result in a large volume of writes.
As a quick test, could you disable watchdog (Database Logging module) and PHP error logging? Then try the import again to see if there are improvements?
As a potential solution for this I am thinking we make the "additionalFields" handling optional for each feed via config. That would allow users to opt in to additionalFields only if needed.
Additionally the approach you have taken to extend the IcalItem class should allow you to define all expected fields, and hence remove the deprecation warnings.
Please take some time to provide feedback here and let me know if disabling the logging improves the performance at all.
@arunkumark make sure you have followed manual steps outlined in #12. There is manual data removal likely needed before you can uninstall.
Can you take a look at the MR? If this fix works for you I'll merge it in and tag 2.2.1 as the latest release.
Thanks!
Thanks @ryanrobinson_wlu! This was at least partially introduced in 2.2.0 with the "Add additionalProperties to IcalParser" fix.
I've added the missing properties you identified, as well as an exception to the additionalProperties logic to hopefully avoid these moving forward.
I also looked at the docs for " #[AllowDynamicProperties]" in https://php.watch/versions/8.2/dynamic-properties-deprecated but that just feels like hiding a potential issue.
@pfrilling for MR review
ben.hamelin → created an issue.
@tBKoT @rszrama I reviewed the patch in #51. It looks good to me, including the removal of the card_code from privateTempStore once it is no longer needed. I believe the need for this approach has been outlined throughout the comments, albeit these are dated and I'm not up to date on the latest Authorize.net API and documentation.
Committed and released with 2.2.0.
Thanks @KarlShea!!
@KarlShea - looks like we got that release in 3.4.0
https://github.com/u01jmg3/ics-parser/releases/tag/v3.4.0
We could plan to up the requirement here and release this as 2.2.0
ben.hamelin → created an issue.
@othermachines agreed, it boils down to what Gatsby needs to know in order to keep the site updated. That will vary from site to site, for example we don't build the menu with Gatsby but we do need paragraph and media data.
It's also worth reviewing this issue: https://www.drupal.org/project/gatsby/issues/3419600 💬 Fastbuilds logic and documentation update Needs review
I've proposed documentation updates to better align with actual module behavior. The "Build published" is another example of misleading or inaccurate docs. It's not checking for published status, but rather restricting to "NodeInterface" when that is checked. It doesn't make sense, and in general the docs seem more targeted at users who are leveraging Gatsby Cloud. For more custom solutions (we build and host our Gatsby site in a custom cloud environment) the docs and logic don't align.
I suggest unchecking the "Build Published" setting and then saving your menu item, to see if it shows up in the fatsbuild logs. If it does, it should get picked up by your next build.
Hi @lindsay.wils!
Please try the patch in #37 or #39. We're using 37 on our affected project.
Longer term we need to open a MR, get review and approval from the maintainers on this fix. It's been a while since I originally dug into this, but at that time I outlined the problem as best I could in comments #14 and #20. But I haven't revisited it since so don't know the current status of the Auth API.
Thanks for this work! We were seeing some PHP errors on occasion:
TypeError: Drupal\gatsby\GatsbyEntityLogger::getJson(): Argument #3 ($recursive) must be of type bool, null given, called in /var/www/html/web/modules/contrib/gatsby/src/GatsbyEntityLogger.php on line 82 in Drupal\gatsby\GatsbyEntityLogger->getJson() (line 247 of /var/www/html/web/modules/contrib/gatsby/src/GatsbyEntityLogger.php).
Added a test to resolve that here.
@dudrop We were seeing similar behavior for any non-node bundle with the "Build Published" setting enabled. If you look at the logic in the gatsby_entity_update() hook: https://git.drupalcode.org/project/gatsby/-/blob/2.0.x/gatsby.module?ref...
it's saying "If build published is set and this is not a node, don't do anything" which doesn't make sense to me logically but does explain why your entity changes aren't showing.
I would suggest trying to make your changes with the "build published" setting disabled. You should then see your entities in the logs at /admin/config/services/gatsby/fastbuilds/logs
Description in #13, #16 were our problem (no wrapper around {{ children }})
This fixed for us in custom theme template container.html.twig:
<div{{ attributes.addClass(classes) }}>{{ children }}</div>
ben.hamelin → created an issue.
ben.hamelin → made their first commit to this issue’s fork.
Just surfacing the change record here as it took a bit to dig out and hoping this helps someone!
https://www.drupal.org/node/3255994 →
I did not dig in to this much, but just noting that during an recent upgrade to 10.1 (from 9.5) this patch was breaking suggestions for views - existing template overrides (e.g. views-view-list--[machine_name]) were not being used. Removing the patch resolved for me.
I ended up following steps outlined in #2 to require this issue branch project directly. We have not tried the recent patches in #53
Thanks for these recent troubleshooting notes. In my case, the upgrade to simplesamlphp/composer-module-installer would not apply for some reason, so the deletion of /vendor/ after having set the new requirement on it to ^1.3 finally worked.
@DieterHolvoet Code review there looks good to me!
This patch / MR is against the development branch 1.0.x - there is no 1.1.x branch to match that tagged release. The tag for 1.1.0 is an older commit, and I couldn't get the patch to apply there.
We ideally would get a D10 release tagged with all updated code.
ben.hamelin → created an issue.
This issue fork is stale, I'm going to open new issue, forked from 1.1.0, and get this fix in.
ben.hamelin → made their first commit to this issue’s fork.
I am guessing this was an issue with the site we first encountered the issue on. It was a rather large site (60,000+ nodes) that had been migrated from D7 and had some legacy code and data. I'm not sure what led to the initial error but this definitely fixed the issue for us.
Happy to let this close out if you feel it's not worthwhile.