The format_date process plugin provided by core isn't very good at parsing different date formats allowed by the ical format in one go. For example, each of these is allowed by ical but it's harder to process them in one config option:

  • 20150116T010000Z
  • 20190903T210000
  • 20180212
CommentFileSizeAuthor
#8 migrate_source_ical-n3187946-8.patch3.8 KBdamienmckenna
#7 migrate_source_ical-n3187946-7.patch3.8 KBdamienmckenna
#7 migrate_source_ical-n3187946-7.interdiff.txt5.76 KBdamienmckenna
#6 3188753-2.patch6.27 KBpaulbanjawan
#3 3188753-1.patch3.64 KBpaulbanjawan

Issue fork migrate_source_ical-3187946

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

DamienMcKenna created an issue. See original summary.

paulbanjawan made their first commit to this issue’s fork.

paulbanjawan’s picture

paulbanjawan commented
StatusFileSize
new 3188753-1.patch3.64 KB
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Active » Needs review

Good work, paulbanjawan, thank you. Let's see if the testbot is set up.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Needs work

I spotted some minor things that need work:

  • Coding standards:
    • There should be a blank line between the "namespace" and "use" statements.
    • There should be a blank line between the "use" statements and the annotation.
    • The setDateFormat() function needs a docblock comment explaining what it does, its @param and its @return.
    • Much of the code of setDateFormat() needs to be reformatted, e.g. spaces after the "if", "elseif" and "else" keywords, and spaces before all open braces.
  • I think the plugin should have a more specific name, e.g. "FormatDateFromIcal" or "IcalFormatDate" with a corresponding ID.
  • The annotation should have a @see line referencing the original class.
  • The annotation needs some documentation explaining how to use it; this can be copied from the original plugin, with appropriate changes.
  • I think it'd make more sense if setDateFormat() was named something related to what it does, e.g. "identifyInputFormat()".
  • It might be useful to clearly indicate in the function which parts were changed from the source plugin, to help future maintainers.
paulbanjawan’s picture

paulbanjawan commented
StatusFileSize
new 3188753-2.patch6.27 KB

Updated the patch based on the last comment of Damien

  • Improved coding standard
  • changed setDateFormat() to identifyInputFormat ()
  • changed the plugin name to IcalFormatDate and plugin id to ical_format_date
  • Added a docblock for function identifyInputFormat
  • Added annotation for the plugin
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review
StatusFileSize
new migrate_source_ical-n3187946-7.interdiff.txt5.76 KB
new migrate_source_ical-n3187946-7.patch3.8 KB

I realized that the entire thing could be boiled down to almost nothing, and let the parent plugin handle most of the work.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
StatusFileSize
new migrate_source_ical-n3187946-8.patch3.8 KB

Correct path - "src/Plugin/migrate/process" instead of "src/Plugin/migrate/Process".

das.gautam made their first commit to this issue’s fork.

das.gautam opened merge request !6

  • das.gautam committed 8eec1d7b on 8.x-1.x 
    Issue #3187946 by paulbanjawan, damienmckenna, das.gautam: Provide a...
das.gautam’s picture

das.gautam commented
das.gautam’s picture

das.gautam commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.