Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add image tag to shortcode task #227

Merged
merged 42 commits into from
May 6, 2019

Conversation

bergice
Copy link
Contributor

@bergice bergice commented Mar 29, 2019

Finds old img/a tags referencing assets and rewrites them to use shortcodes

Depends on silverstripe/silverstripe-framework#8892

Extra tests to add

  • Test that live content gets updated as well
  • Test that other dataobjects
  • Test DB fields not called content
  • Test an HTMLVarchar
  • Test scenarios where files have been migrated
  • Test scenarios where files are protected

@bergice bergice force-pushed the pulls/1/img-to-shortcode branch 12 times, most recently from 385bc38 to c7f4428 Compare April 8, 2019 03:26
@bergice bergice marked this pull request as ready for review April 10, 2019 23:04
src/Dev/Tasks/TagsToShortcodeHelper.php Show resolved Hide resolved
src/Dev/Tasks/TagsToShortcodeHelper.php Outdated Show resolved Hide resolved
src/Dev/Tasks/TagsToShortcodeHelper.php Show resolved Hide resolved
src/Dev/Tasks/TagsToShortcodeHelper.php Outdated Show resolved Hide resolved
src/Dev/Tasks/TagsToShortcodeHelper.php Outdated Show resolved Hide resolved
src/Dev/Tasks/TagsToShortcodeHelper.php Outdated Show resolved Hide resolved
src/Dev/Tasks/TagsToShortcodeHelper.php Outdated Show resolved Hide resolved
src/Dev/Tasks/TagsToShortcodeTask.php Outdated Show resolved Hide resolved
src/Dev/Tasks/TagsToShortcodeTask.php Show resolved Hide resolved
src/Dev/Tasks/TagsToShortcodeHelper.php Outdated Show resolved Hide resolved
Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some bits of feedback.

The unit tests needs to be updated to be a bit more beefy. e.g.:

  • links to hash path
  • link to natural path
  • link to variants of any path
  • link to external files
  • broken html
  • image tag with and without closing bracket (<img/> as opposed to <img>)
  • image tag inside a link
  • attributes with single quotes
  • attributes without quotes
  • bad casing for tags or attributes
  • images and links that should not be updated

src/Dev/Tasks/TagsToShortcodeHelper.php Show resolved Hide resolved
src/Dev/Tasks/TagsToShortcodeHelper.php Outdated Show resolved Hide resolved

/**
* Get all tags within some page content and return as array.
* @param $content
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like this has been fixed.

src/Dev/Tasks/TagsToShortcodeHelper.php Outdated Show resolved Hide resolved
@bergice bergice self-assigned this Apr 23, 2019
@unclecheese unclecheese changed the base branch from 1 to 1.4 April 24, 2019 02:46
bergice and others added 9 commits April 29, 2019 12:48
Finds old `img` tags referencing assets and rewrites them to use shortcodes
- `TagsToShortcode` now also rewrites `a` tags instead of just `img` tags
- Fix travis build
		- baseClass: The base class that will be used to look up HTMLText fields. Defaults to SilverStripe\ORM\DataObject
		- includeBaseClass: Whether to include the base class' HTMLText fields or not
@maxime-rainville maxime-rainville dismissed chillu’s stale review May 2, 2019 23:08

Everything has changed

@maxime-rainville
Copy link
Contributor

Test won't be green until silverstripe/silverstripe-framework#8960 is merge.

chillu pushed a commit to silverstripe/silverstripe-framework that referenced this pull request May 6, 2019
@chillu
Copy link
Member

chillu commented May 6, 2019

Was it an explicit decision not to rewrite _versions tables here? It means the problem will be reintroduced whenever somebody rolls back to an older version. And there's some inconsistency between the "latest version" for draft and live stages, and the content in the draft and live tables.

If we're not planning to fix that, it's definitely a gotcha we should mention in the upgrade guide. This is modifying people's data, it's absolutely essential that they understand what's happening.

$tagsToShortcodeHelper = new TagsToShortcodeHelper();
$tagsToShortcodeHelper->run();

/** @var SiteTree $newPage */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a sidenote: I think this isn't great usage of fixtures - it makes it quite hard to see the before/after state. I generally try to avoid fixtures where the data of those records is relevant to understanding the tests, and default to creating them inline. No need to change here, just thought it's worth mentioning.

@maxime-rainville
Copy link
Contributor

maxime-rainville commented May 6, 2019

@chillu We asked the questions in a stand up a wee while ago and you said it wasn't worth it, so we didn't do it (that will be my go-to excuse from now on ... "we told you about it in some unspecified meeting a few months ago").

If you think it's worthwhile, it would not be a big deal to apply it to previous versions as well, although it becomes a bit more murky because some of the files might not be there any more or the DB structure could have changed somewhere in-between. Also, depending on how many previous versions they are, it could take a lot longer to apply the changes.

$documentID = $this->idFromFixture(File::class, 'document');
$imageID = $this->idFromFixture(Image::class, 'image1');

$this->assertContains(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each of these assertions should spell out what they're testing

@maxime-rainville
Copy link
Contributor

Grrr ... we still have test failings!!! I'll fix those.

@maxime-rainville
Copy link
Contributor

Those unit test should be working now.

@unclecheese unclecheese merged commit 739119f into silverstripe:1.4 May 6, 2019
@unclecheese unclecheese deleted the pulls/1/img-to-shortcode branch May 6, 2019 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants