From cab28448d0a24a0ca99b2da745134e010cd9a266 Mon Sep 17 00:00:00 2001
From: Leonard Richardson <leonardr@segfault.org>
Date: Mon, 31 Dec 2018 11:32:26 -0500
Subject: Improved and tested error checking for insert_before and
 insert_after.

---
 bs4/element.py         | 27 ++++++++++++++-------------
 bs4/tests/test_tree.py | 16 ++++++++++++++++
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/bs4/element.py b/bs4/element.py
index c959c3e..c367dee 100644
--- a/bs4/element.py
+++ b/bs4/element.py
@@ -451,13 +451,13 @@ class PageElement(object):
         The elements will have the same parent, and the given elements
         will be immediately before this one.
         """
-        for predecessor in args:
-            if self is predecessor:
+        parent = self.parent
+        if parent is None:
+            raise ValueError(
+                "Element has no parent, so 'before' has no meaning.")
+        if self in args:
                 raise ValueError("Can't insert an element before itself.")
-            parent = self.parent
-            if parent is None:
-                raise ValueError(
-                    "Element has no parent, so 'before' has no meaning.")
+        for predecessor in args:
             # Extract first so that the index won't be screwed up if they
             # are siblings.
             if isinstance(predecessor, PageElement):
@@ -471,15 +471,16 @@ class PageElement(object):
         The elements will have the same parent, and the given elements
         will be immediately after this one.
         """
-
+        # Do all error checking before modifying the tree.
+        parent = self.parent
+        if parent is None:
+            raise ValueError(
+                "Element has no parent, so 'after' has no meaning.")
+        if self in args:
+            raise ValueError("Can't insert an element after itself.")
+        
         offset = 0
         for successor in args:
-            if self is successor:
-                raise ValueError("Can't insert an element after itself.")
-            parent = self.parent
-            if parent is None:
-                raise ValueError(
-                    "Element has no parent, so 'after' has no meaning.")
             # Extract first so that the index won't be screwed up if they
             # are siblings.
             if isinstance(successor, PageElement):
diff --git a/bs4/tests/test_tree.py b/bs4/tests/test_tree.py
index e6c07aa..2290558 100644
--- a/bs4/tests/test_tree.py
+++ b/bs4/tests/test_tree.py
@@ -964,6 +964,14 @@ class TestTreeModification(SoupTest):
         self.assertEqual(
             soup.decode(), self.document_for("QUUX<b>bar</b><a>foo</a>BAZ"))
 
+        # Can't insert an element before itself.
+        b = soup.b
+        self.assertRaises(ValueError, b.insert_before, b)
+
+        # Can't insert before if an element has no parent.
+        b.extract()
+        self.assertRaises(ValueError, b.insert_before, "nope")
+        
     def test_insert_multiple_before(self):
         soup = self.soup("<a>foo</a><b>bar</b>")
         soup.b.insert_before("BAZ", " ", "QUUX")
@@ -985,6 +993,14 @@ class TestTreeModification(SoupTest):
         self.assertEqual(
             soup.decode(), self.document_for("QUUX<b>bar</b><a>foo</a>BAZ"))
 
+        # Can't insert an element after itself.
+        b = soup.b
+        self.assertRaises(ValueError, b.insert_after, b)
+
+        # Can't insert after if an element has no parent.
+        b.extract()
+        self.assertRaises(ValueError, b.insert_after, "nope")
+        
     def test_insert_multiple_after(self):
         soup = self.soup("<a>foo</a><b>bar</b>")
         soup.b.insert_after("BAZ", " ", "QUUX")
-- 
cgit v1.2.1