diff --git a/code/model/SiteTree.php b/code/model/SiteTree.php index b9e0cd78..f228dd5d 100755 --- a/code/model/SiteTree.php +++ b/code/model/SiteTree.php @@ -1612,46 +1612,6 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid return ($liveRecord) ? $liveRecord->URLSegment : null; } - /** - * Rewrites any linked images on this page without creating a new version record. - * Non-image files should be linked via shortcodes - * Triggers the onRenameLinkedAsset action on extensions. - * - * @todo Implement image shortcodes and remove this feature - */ - public function rewriteFileLinks() { - // Skip live stage - if(\Versioned::get_stage() === \Versioned::LIVE) { - return; - } - - // Update the content without actually creating a new version - foreach($this->db() as $fieldName => $fieldType) { - // Skip if non HTML or if empty - if ($fieldType !== 'HTMLText') { - continue; - } - $fieldValue = $this->{$fieldName}; - if(empty($fieldValue)) { - continue; - } - - // Regenerate content - $content = Image::regenerate_html_links($fieldValue); - if($content === $fieldValue) { - continue; - } - - // Write content directly without updating linked assets - $table = ClassInfo::table_for_object_field($this, $fieldName); - $query = sprintf('UPDATE "%s" SET "%s" = ? WHERE "ID" = ?', $table, $fieldName); - DB::prepared_query($query, array($content, $this->ID)); - - // Update linked assets - $this->invokeWithExtensions('onRenameLinkedAsset'); - } - } - /** * Returns the pages that depend on this page. This includes virtual pages, pages that link to it, etc. * @@ -2181,13 +2141,13 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid ); } } else { - if($this->canDelete()) { - // delete - $moreOptions->push( - FormAction::create('delete',_t('CMSMain.DELETE','Delete draft')) - ->addExtraClass('delete ss-ui-action-destructive') - ); - } + if($this->canDelete()) { + // delete + $moreOptions->push( + FormAction::create('delete',_t('CMSMain.DELETE','Delete draft')) + ->addExtraClass('delete ss-ui-action-destructive') + ); + } if($this->canArchive()) { // "archive" $moreOptions->push( @@ -2240,7 +2200,7 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid WHERE EXISTS (SELECT "SiteTree"."Sort" FROM "SiteTree" WHERE "SiteTree_Live"."ID" = "SiteTree"."ID") AND "ParentID" = ?', array($this->ParentID) ); - } + } /** * Update draft dependant pages diff --git a/code/model/SiteTreeFileExtension.php b/code/model/SiteTreeFileExtension.php index 8fb5cade..bc21705d 100644 --- a/code/model/SiteTreeFileExtension.php +++ b/code/model/SiteTreeFileExtension.php @@ -124,43 +124,4 @@ class SiteTreeFileExtension extends DataExtension { } } } - - public function onAfterWrite() { - // Update any database references in the current stage - $this->updateLinks(); - } - - public function onAfterVersionedPublish() { - // Ensure that ->updateLinks is invoked on the draft record - // after ->doPublish() is invoked. - $this->updateLinks(); - } - - /** - * Rewrite links to the $old file to now point to the $new file. - * - * @uses SiteTree->rewriteFileID() - */ - public function updateLinks() { - // Skip live stage - if(\Versioned::get_stage() === \Versioned::LIVE) { - return; - } - - if(class_exists('Subsite')) { - Subsite::disable_subsite_filter(true); - } - - $pages = $this->owner->BackLinkTracking(); - if($pages) { - foreach($pages as $page) { - $page->rewriteFileLinks(); - } - } - - if(class_exists('Subsite')) { - Subsite::disable_subsite_filter(false); - } - } - } diff --git a/code/model/SiteTreeLinkTracking.php b/code/model/SiteTreeLinkTracking.php index 3ce7ab60..3ecc1773 100644 --- a/code/model/SiteTreeLinkTracking.php +++ b/code/model/SiteTreeLinkTracking.php @@ -93,8 +93,6 @@ class SiteTreeLinkTracking extends DataExtension { /** * Scrape the content of a field to detect anly links to local SiteTree pages or files * - * @todo - Replace image tracking with shortcodes - * * @param string $fieldName The name of the field on {@link @owner} to scrape */ public function trackLinksInField($fieldName) { @@ -108,6 +106,11 @@ class SiteTreeLinkTracking extends DataExtension { // Highlight broken links in the content. foreach ($links as $link) { + // Skip links without domelements + if(!isset($link['DOMReference'])) { + continue; + } + $classStr = trim($link['DOMReference']->getAttribute('class')); if (!$classStr) { $classes = array(); @@ -142,6 +145,7 @@ class SiteTreeLinkTracking extends DataExtension { break; case 'file': + case 'image': if ($link['Broken']) { $record->HasBrokenFile = true; } else { @@ -157,22 +161,6 @@ class SiteTreeLinkTracking extends DataExtension { } } - // Add file tracking for image references - if($images = $htmlValue->getElementsByTagName('img')) foreach($images as $img) { - // {@see HtmlEditorField} for data-fileid source - $fileID = $img->getAttribute('data-fileid'); - if(!$fileID) { - continue; - } - - // Assuming a local file is linked, check if it's valid - if($image = File::get()->byID($fileID)) { - $linkedFiles[] = $image->ID; - } else { - $record->HasBrokenFile = true; - } - } - // Update the "LinkTracking" many_many if($record->ID && $record->manyManyComponent('LinkTracking') && ($tracker = $record->LinkTracking())) { $tracker->removeByFilter(array( @@ -274,15 +262,15 @@ class SiteTreeLinkTracking_Parser { // Link to a page on this site. $matches = array(); - if(preg_match('/\[sitetree_link(?:\s*|%20|,)?id=([0-9]+)\](#(.*))?/i', $href, $matches)) { - $page = DataObject::get_by_id('SiteTree', $matches[1]); + if(preg_match('/\[sitetree_link(?:\s*|%20|,)?id=(?[0-9]+)\](#(?.*))?/i', $href, $matches)) { + $page = DataObject::get_by_id('SiteTree', $matches['id']); $broken = false; if (!$page) { // Page doesn't exist. $broken = true; - } else if (!empty($matches[3])) { - $anchor = preg_quote($matches[3], '/'); + } else if (!empty($matches['anchor'])) { + $anchor = preg_quote($matches['anchor'], '/'); if (!preg_match("/(name|id)=\"{$anchor}\"/", $page->Content)) { // Broken anchor on the target page. @@ -292,8 +280,8 @@ class SiteTreeLinkTracking_Parser { $results[] = array( 'Type' => 'sitetree', - 'Target' => $matches[1], - 'Anchor' => empty($matches[3]) ? null : $matches[3], + 'Target' => $matches['id'], + 'Anchor' => empty($matches['anchor']) ? null : $matches['anchor'], 'DOMReference' => $link, 'Broken' => $broken ); @@ -303,13 +291,13 @@ class SiteTreeLinkTracking_Parser { // Link to a file on this site. $matches = array(); - if(preg_match('/\[file_link(?:\s*|%20|,)?id=([0-9]+)\]/i', $href, $matches)) { + if(preg_match('/\[file_link(?:\s*|%20|,)?id=(?[0-9]+)/i', $href, $matches)) { $results[] = array( 'Type' => 'file', - 'Target' => $matches[1], + 'Target' => $matches['id'], 'Anchor' => null, 'DOMReference' => $link, - 'Broken' => !DataObject::get_by_id('File', $matches[1]) + 'Broken' => !DataObject::get_by_id('File', $matches['id']) ); continue; @@ -331,6 +319,19 @@ class SiteTreeLinkTracking_Parser { } + // Find all [image ] shortcodes (will be inline, not inside attributes) + $content = $htmlValue->getContent(); + if(preg_match_all('/\[image([^\]]+)\bid=(["])?(?\d+)\D/i', $content, $matches)) { + foreach($matches['id'] as $id) { + $results[] = array( + 'Type' => 'image', + 'Target' => (int)$id, + 'Anchor' => null, + 'DOMReference' => null, + 'Broken' => !DataObject::get_by_id('Image', (int)$id) + ); + } + } return $results; } diff --git a/tests/model/FileLinkTrackingTest.php b/tests/model/FileLinkTrackingTest.php index 63e09b79..11915ace 100644 --- a/tests/model/FileLinkTrackingTest.php +++ b/tests/model/FileLinkTrackingTest.php @@ -28,7 +28,7 @@ class FileLinkTrackingTest extends SapphireTest { $imageID = $this->idFromFixture('Image', 'file1'); $page = $this->objFromFixture('Page', 'page1'); $page->Content = sprintf( - '

', + '

[image src="/assets/FileLinkTrackingTest/55b443b601/testscript-test-file.jpg" id="%d"]

', $imageID ); $page->write(); @@ -39,20 +39,28 @@ class FileLinkTrackingTest extends SapphireTest { parent::tearDown(); } + /** + * Test uses global state through Versioned::set_reading_mode() since + * the shortcode parser doesn't pass along the underlying DataObject + * context, hence we can't call getSourceQueryParams(). + */ public function testFileRenameUpdatesDraftAndPublishedPages() { $page = $this->objFromFixture('Page', 'page1'); $page->doPublish(); // Live and stage pages both have link to public file + Versioned::set_stage(Versioned::DRAFT); $this->assertContains( 'ID))->value() + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); + Versioned::set_stage(Versioned::LIVE); $this->assertContains( 'ID))->value() + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); + Versioned::set_stage(Versioned::DRAFT); $file = $this->objFromFixture('Image', 'file1'); $file->Name = 'renamed-test-file.jpg'; $file->write(); @@ -60,38 +68,43 @@ class FileLinkTrackingTest extends SapphireTest { // Staged record now points to secure URL of renamed file, live record remains unchanged // Note that the "secure" url doesn't have the "FileLinkTrackingTest" component because // the mocked test location disappears for secure files. + Versioned::set_stage(Versioned::DRAFT); $this->assertContains( 'ID))->value() + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); + Versioned::set_stage(Versioned::LIVE); $this->assertContains( 'ID))->value() + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); // Publishing the file should result in a direct public link (indicated by "FileLinkTrackingTest") // Although the old live page will still point to the old record. // @todo - Ensure shortcodes are used with all images to prevent live records having broken links $file->doPublish(); + Versioned::set_stage(Versioned::DRAFT); $this->assertContains( 'ID))->value() + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); + Versioned::set_stage(Versioned::LIVE); $this->assertContains( - // Note: Broken link until shortcode-enabled - 'ID))->value() + 'byID($page->ID)->dbObject('Content')->forTemplate() ); - // Publishing the page after publishing the asset will resolve any link issues + // Publishing the page after publishing the asset should retain linking $page->doPublish(); + Versioned::set_stage(Versioned::DRAFT); $this->assertContains( 'ID))->value() + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); + Versioned::set_stage(Versioned::LIVE); $this->assertContains( 'ID))->value() + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); } @@ -112,20 +125,20 @@ class FileLinkTrackingTest extends SapphireTest { $file->write(); // Verify that the draft virtual pages have the correct content - $svp = Versioned::get_by_stage('VirtualPage', Versioned::DRAFT)->byID($svp->ID); + Versioned::set_stage(Versioned::DRAFT); $this->assertContains( 'Content + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); // Publishing both file and page will update the live record $file->doPublish(); $page->doPublish(); - $svp = Versioned::get_by_stage('VirtualPage', Versioned::LIVE)->byID($svp->ID); + Versioned::set_stage(Versioned::LIVE); $this->assertContains( 'Content + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); } @@ -151,11 +164,15 @@ class FileLinkTrackingTest extends SapphireTest { public function testTwoFileRenamesInARowWork() { $page = $this->objFromFixture('Page', 'page1'); $this->assertTrue($page->doPublish()); + + Versioned::set_stage(Versioned::LIVE); $this->assertContains( 'ID))->value()); + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() + ); // Rename the file twice + Versioned::set_stage(Versioned::DRAFT); $file = $this->objFromFixture('Image', 'file1'); $file->Name = 'renamed-test-file.jpg'; $file->write(); @@ -168,16 +185,18 @@ class FileLinkTrackingTest extends SapphireTest { $file->doPublish(); // Confirm that the correct image is shown in both the draft and live site + Versioned::set_stage(Versioned::DRAFT); $this->assertContains( 'ID))->value() + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); // Publishing this record also updates live record $page->doPublish(); + Versioned::set_stage(Versioned::LIVE); $this->assertContains( 'ID))->value() + Page::get()->byID($page->ID)->dbObject('Content')->forTemplate() ); } } diff --git a/tests/model/FileLinkTrackingTest.yml b/tests/model/FileLinkTrackingTest.yml index c441a251..6c061472 100644 --- a/tests/model/FileLinkTrackingTest.yml +++ b/tests/model/FileLinkTrackingTest.yml @@ -9,4 +9,4 @@ Page: page1: Title: page1 URLSegment: page1 - Content: '

' + # Content is set via test setup \ No newline at end of file diff --git a/tests/model/SiteTreeHTMLEditorFieldTest.php b/tests/model/SiteTreeHTMLEditorFieldTest.php index 91530d1a..b8d66602 100644 --- a/tests/model/SiteTreeHTMLEditorFieldTest.php +++ b/tests/model/SiteTreeHTMLEditorFieldTest.php @@ -124,7 +124,7 @@ class SiteTreeHtmlEditorFieldTest extends FunctionalTest { $editor = new HtmlEditorField('Content'); $file = $this->objFromFixture('Image', 'example_image'); - $editor->setValue(sprintf('', $file->getURL(), $file->ID)); + $editor->setValue(sprintf('[image src="%s" id="%d"]', $file->getURL(), $file->ID)); $editor->saveInto($sitetree); $sitetree->write(); $this->assertEquals( diff --git a/tests/model/SiteTreeLinkTrackingTest.php b/tests/model/SiteTreeLinkTrackingTest.php index 7ea1ff5f..64b9ca3d 100644 --- a/tests/model/SiteTreeLinkTrackingTest.php +++ b/tests/model/SiteTreeLinkTrackingTest.php @@ -62,8 +62,8 @@ class SiteTreeLinkTrackingTest extends SapphireTest { } public function testHasBrokenFile() { - $this->assertTrue($this->pageIsBrokenFile('')); - $this->assertFalse($this->pageIsBrokenFile('')); + $this->assertTrue($this->pageIsBrokenFile('[image src="someurl.jpg" id="99999999"]')); + $this->assertFalse($this->pageIsBrokenFile('[image src="someurl.jpg"]')); } protected function pageIsBrokenFile($content) {