Optimize Drupal\Core\Template\Attribute

Disagree because just as classes must not try to do too much, so too should individual tasks. The Assertion Handling ticket deals specifically with the resolution of the assertions. The Assertion tickets themselves, like this one, may end up numbering well over a hundred. We're talking about going over the entire Drupal 8 codebase over the next few years and finding spots where it will help our code development to have an assert statement. If all that material was bound to the Assertion Handling ticket that ticket would never be completed.

The scope of this may need to be expanded to handle the whole Template Core library though. An individual class likely is too specific, but I'm in a baby steps mentality.

Ah. Makes sense. A good candidate for that would be the boot assertions.

The template assertions and the explanation of the services.yml snafu that inspired this whole project.

This patch can (and should) be tested without its parent however, if it fails it may be something other than the patch at fault. All I've done is insert assertion statements based on the PHPdoc lines and the assertions regarding the url and link generators. The testers run in ASSERT_WARN mode, and if they trip then either something is wrong with the test, or something is wrong with the code outside the assertion (that I'm not qualified to fix), or the PHP doc statements are wrong and least likely is the assertion is wrong.

Without it's parent there is no way for these assertions to become active and be trapped in actual use.

Again, these materials are here off in their own file to keep the parent from becoming monstrous in size.

Updated issue summary to conform with template.

The correction of these fails is a separate issue and will be reported as such. The assertion in question is working as designed assuming the php doc is correct.

This includes the patch at the tail of this ticket..

https://www.drupal.org/node/2449741

That fulfills both the failed tests and the assertion.

Issue summary updated as requested.

Lot of changes, need to get a handle on what breaks on a system wide test. This patch is not up to snuff comments or code formatting wide - I just need to see how the test bots handle it. Also note - this particular patch is combined with it's parent. I'm unsure if I want to keep them divided any longer.

Added Beta eval.

Last patch had a require statement I was using to pull var-dumper into PHP unit test I forgot to drop...

Couple of fixes which locally look to be accounting for a least 3/4 of the errors - a bad regex and a failure to init the class array.

The last two I've tried to make had so many whitespace switchout notices I could understand what they meant and I wrote the code. The next patch I push will be properly formatted and will be the last one without an interdiff against previous patches, I swear. It will have an interdiff against itself and it's test code.

I'm not at a patch release point tonight as I end work, but this is what has been added.

1. TWIG. That should make themers happy. For the moment it is using its own internal templates and it starts a raw copy of twig with no Drupal extensions whatsoever. I plan to allow it to use the current theme's templates if I can accurately determine it is safe to do so. That or use an independent means of informing the fault system on were to find errors. Input on which to do appreciated.

2 Namespace changed. From Drupal\Core\Error to Drupal\Core\Fault Since "Error" refers to PHP's older trigger_error system it's a misleading name for the system. Fault however can apply to any of the three types.

3 Symfony Responder and Request are now used to compose the handler responses for consistency with the rest of the system. Note this means the fault system will require a clean copy of Symfony to work. It is not perturbed by issues in Drupal though. The assertion callback is now in a child class of Symfony's Response class.

4. The core of the parent system will be in three classes. Except for minor changes and additions, they are done.

# Additions I must make
I must get the system to close gracefully when Twig has a compile error against its own files.
Extensions to the unit tests

1. PHP Unit, as of the last test, can check assertions. Simpletest needs the same functionality so that clean tests can be written.

# Additions I'm mulling over as feasable in time for Drupal 8.0.x release

Translation file loading - this will work by simply looking in sites/default for a faults.yml file (formerly error.yml). That file can be in any language desired, and it's responses override those found elsewhere in the system. A method will be exposed that will be callable from the settings.php file to point the FaultHandler at a different local faults.yml file.

INPUT NEEDED!! Deprecation declaration - I know how to modify Drupal's base unit test class in PHPUnit and I believe simpletest to ignore trigger_error(' ', E_USER_DEPRECATED). This would allow deprecation throws to be put onto those methods scheduled for removal. The main ramification of this is production sites would need to run as E_ALL & ~E_USER_DEPRECATED instead of just E_ALL. Likewise the autotesters would have to be set up to note deprecation throws but not fail the test over them. Also, instead of coming up with error messages for these, the message string for them would be the projected removing version - for example

trigger_error('8.0rc1', E_USER_DEPRECATED)

The @deprecated php comment lets us know what's deprecated, but it doesn't let us know who's calling it. Clearing the way for that error throw solves this problem. Then again, maybe that should just get it's own ticket. Thoughts?

I'm to the unit tests now, other problems are resolved - I think - though I won't *know* until the tests are running.

More changes

* Unrelated code from the Fault System library removed from this patch. What remains is what is needed by this patch's unit tests.
* The Attribute class has been converted to an array object - it was already implementing all of the interfaces. PHP's internal implementation of Array Object will run faster than the homemade one that has been replaced. The storage method now merely wraps copyArray to maintain BC.
* Class name validity assertion moved from the ValueBase class to Attribute.
* Null can now be passed to an attribute.
* Code formatting applied.

I was still having trouble with some tests locally unrelated - but since those same tests fail when I checkout head locally I'm going to see if it's just a local problem.

Checking something. This patch should pass. If it don't, head is broken.

This should pass. The problem was the test suite requires XHTML attribute format to validate.

The tests that failed did so because they where checking for HTML5 style boolean attributes. The Attribute system cannot reliably be asked to provide alternating formats, and replacing XPath promises to be more difficult so the values these tests are looking for as been changed to XHTML.

Better title added.

Reminder, the patch above contains enough dupe code from the parent ticket to pass its own unit tests since those tests cannot be conducted without that supporting code.

But that would be patch 29. Should I re-upload it with that label?

Updated the template patch to reflect the changes in its parent and the 7.0 compat changes which make the previous patch unappliable. As mentioned previously I had a git crash which lost my change record so I can't do an interdiff with the previous patch.

The do-not-test patch contains just the code relevant to this ticket. It isn't testable on its own though - the testing ticket includes the parent issue.

Somehow the do not test patch ran when it wasn't supposed to. Full patch passed.

It is faster but it does nothing to the incoming array, and neither does parent::_construct(). Take a closer look at offsetSet

  /**
   * Overrides ArrayObject::offsetSet().
   */
  public function offsetSet($name, $value) {
    parent::offsetSet($name, $this->createAttributeValue($name, $value));
  }

There is a method to my madness up there - by doing that loop I force every value to pass through the validation and transformation performed by createAttributeValue.

Any call set done via $this[$var] = 'foo' will pass through offsetSet magically because of the ArrayAccess Interface.

Ok, new class (And I guess a very minor API addition) added -- AttributeNamedId. This class monitors attributes named Id's, making sure they are of a valid format to be ID's, and making sure they are unique document wide (or in the case of Ajax responses they are at least unique within the dataset being transferred). These assertions will help diagnose certain JavaScript bugs - browser behavior when one or more elements share an ID is undefined.

There are three patches. The one that is being tested includes code from the parent ticket needed for testing to occur. Then there is the main template assertion patch for those just now reading the ticket. For those following along there is an interdiff with the version on post #36.

EDIT: Just noticed I didn't do a code standard sniff. I'll await the results of global unit testing to make that check.

SECOND EDIT: It occurred to me this class has the potential to issue random ID's safely to doc elements. I've seen jQuery and prototype both do this from time to time. Is there a usage scenario for this? The way I would implement this is simply $attribute['id]->randomize() with the function returning the assigned ID. Before I place that logic though I'd like to know a use case for it because at the moment I can't think of any useful application which makes it code bloat. Also, the presence of the feature would force the ID's issued to need to be tracked in production.

I've double checked the failure output against several of the pages the fail is reported when the patch is *not* installed - there are indeed multiple pages with repeated id's in the system. Hence the AttributeNamedId class cannot be added to the system until these errors are removed.

A separate issue has been created for this problem.

Updating issue tags now that I have a better understanding on what they should be.

Also changing back to Needs Review. While the id issue is decided the previous working patch without AttributeNamedId (#36) can still be committed.

Checking on exactly how many test fails was solved by fixing the simpletest master page id duping. If this works I'll do a more formal interdiff post.

As of this post, dividing the ticket. The Attribute work runs deeper than I anticipated as the newbie coming on board, and far out scopes what this ticket was originally meant for - simply adding some asserts to TwigExtension. So the patches attached address just that problem which was the original goal of this ticket. The assertion work will move to a new ticket I'll link up from here when it's ready, since it's massive.

The "asserts-only" is just this ticket's code. The other ticket includes supporting code from the parent ticket.

This is a start over. No assertion aware tests have been added to this ticket yet - they may not be needed since all the assertions are of the very straightforward 'is_a' variety. We do check to see if the urlGenerator is set.

Any assertions that fail on this preliminary search will be assigned their own child tickets and removed from this one. PHP Units have passed.

There is (and will be) no work on the Attribute classes in this ticket anymore.

1 is just a tiny optimization - I'll roll it back if desired. 2. is a code standards violation - I can't get phpcs to validate the changes unless I do that. 3 I can fix (I hadn't ran phpcs over the file yet.

Changes made.

Yes. This particular assertion group doesn't need it's parent, but I accidentally merged it in anyway.

With the parent now in core, this should be a final re-roll. A followup ticket will adjust TwigExtensionTest to actually test the assertions being added here, but those assertion tests require #2536560: Runtime Assertion unit and functional testing to run.

NOTE: The .htaccess file in the patch has the assert flag set to 1 due to an issue with the test bots. This file change should not be included in the final commit to core.

Reuploading patch to force the test bots to eval.

I forgot to note in this ticket that I'm splitting it up by class in attempt to speed things along. Should be able to get some work in on this after work (in about 12 hours)

Environment work split to #2555549: Assertions in \Drupal\Template\TwigEnvironment

Extensions work split to #2558079: Assertions in \Drupal\Template\TwigExtension

Both sub tickets are ready, but Alex Pott stated in IRC he doesn't want to introduce any further assertions during the RC cycle. So 8.0.1 I guess is earliest possible - I dunno for sure.

Both child ticket patches for this work with 8.2.x, so I'm not sure this necessarily needs further postponement - it can probably be worked in.

I'm working on this again, stripping out the disruptive stuff and leaving behind the optimizations.

Ok, new patch. The scope of this patch has been changed to just the optimizations for the Attribute classes. Twig related stuff will be handled off in their own patch.

I massively rewrote the issue summary so please check that.

Ok, Let's try that again with the Safe Markup branch of createAttributeValue properly implemented. Hopefully this test won't look like an explosion in a mattress factory.

Dropping AttributeBoolean and AttributeString. The former because it's gross overkill, the Attribute class can handle it with less overhead and lines of code than having a new class. The latter because its misleading - String was the catch all for anything not handled by another class since Attributes are written as string. The logic for this is now in Attribute class.

Render is removed from the interface and all classes. Attribute is responsible for rendering. This allows these helper classes to be ignorant of their names which cleans up the copying code of Attributes between elements.

Seeing if the file name itself is the reason the patch won't work.

Trying creating the patch from the command line rather than using sourcetree.

88 is going to fail, so here's 90. Git on Windows puts garbage before and after the diff calls, so I trimmed out the garbage. Maybe this will apply. Stupid Windows.

Another try. At least the patch is applying now.

Perhaps the reason these outside tests are failing is I'm trying to refactor too much. So the AttributeValueInterfaces for all not null objects are back in place.

Attribute must be able to accept objects with ArrayAccess.

Again.

Ok, I'll do that. This patch only converts Attribute to an ArrayObject. I also modified the remove attributes test to work with the attributes being correctly unset. The test was calling assertEmpty but that isn't correct - Empty means the key is still set but the value is nulled out or an empty string. The original code and the comments imply that a test of whether the key was unset is needed.

The source of the problem was Attribute::removeAttribute() wasn't unsetting the keys.

Ok, concerns above addressed. I forgot that assertArrayNotHasKey would work with an array object. Test coverage also expanded, see interdiff. I added the AttributeValueInterface so that I wouldn't have to turn around and rewrite my new tests in a future patch, but for the moment it's only attached to AttributeValueBase. Eventually it will replace that class.

EDIT: I changed the constructor out since I realized that wrapping must occur after exchangeArray is called as well. The unit tests make it clear why the wrapping must occur. Remember that the original constructor did a foreach loop to apply createAttributeValue as well instead of just piping the submitted array into storage.

QUESTION: Attribute will pick up the default ArrayObject implementation of the serializable interface after this patch. Should we test this in any way?

Updating the issue description to entail exactly what work is being done on this ticket.

I couldn't get the script to run under kalabox so I modified it to run off my site's root using PHP directly. In that configuration the tests still favor the old code, but only by a tenth of a second. Seeing if I can get that last tenth back.

<?php

use Drupal\Core\Template\Attribute;

require_once 'autoload.php';

function providerTestAttributeData() {
  return [
    ['&"\'<>' => 'value'],
    ['title' => '&"\'<>'],
    ['class' => ['first', 'last']],
    ['disabled' => TRUE],
    ['disabled' => FALSE],
    ['alt' => ''],
    ['alt' => NULL],
    [
      'id' => 'id-test',
      'class' => ['first', 'last'],
      'alt' => 'Alternate',
    ],
    [],
  ];
}

$startTime = microtime(true);
// Your content to test
$attributesData = providerTestAttributeData();
for ($i=0; $i < 10000; $i++) {
  foreach ($attributesData as  $attributes) {
    // Convert array to attribute object.
    $attribute = new Attribute($attributes);
    // Change Attribute.
    $attribute->addClass(array('orange', 'blue'));
    $attribute->removeAttribute('id');
    // Cast to string.
    $value = (string) $attribute;
  }
}
$endTime = microtime(true);
$elapsed = $endTime - $startTime;

echo "Execution time : $elapsed seconds";

Whatever you name the script as you can invoke it with the PHP cli.

There was a notice error being raised when attempting to unset a not set element that slowed down the new code noticeably. Old code still edging it out, which doesn't make much sense since ArrayObject is implemented in C++

Here's the interdiff for that.

I'm going to continue with the CSS class modification layer and see if the speed loss here can be gained back in that optimization since a large part of your test is passing through the CSS class code which is also slated for rewrite in this series of patches.

Ok, I've started over, running the speed check with each change. I'm going to move away from implementing \ArrayObject. So far I've picked up a 1/2 second improvement on this test:

<?php

use Drupal\Core\Template\Attribute;

require_once 'autoload.php';

function providerTestAttributeData() {
  return [
    ['&"\'<>' => 'value'],
    ['title' => '&"\'<>'],
    ['class' => ['first', 'last']],
    ['disabled' => TRUE],
    ['disabled' => FALSE],
    ['alt' => ''],
    ['alt' => NULL],
    [
      'id' => 'id-test',
      'class' => ['first', 'last'],
      'alt' => 'Alternate',
    ],
    [],
  ];
}

$startTime = microtime(true);
// Your content to test
$attributesData = providerTestAttributeData();
for ($i=0; $i < 100000; $i++) {
  foreach ($attributesData as  $attributes) {
    // Convert array to attribute object.
    $attribute = new Attribute($attributes);
    // Change Attribute.
    $attribute->addClass(array('orange', 'blue'));
    $attribute->removeAttribute('id');
    // Clone
    $other_attribute = new Attribute($attribute);
    // Cast to string.
    $value = (string) $attribute;
  }
}
$endTime = microtime(true);
$elapsed = $endTime - $startTime;

echo "Execution time : $elapsed seconds";

The largest change to this test is I'm also testing attribute cloning efficiency. That's testing this:

     if ($value instanceof AttributeValueBase) {
-      $class = get_class($value);
-      return new $class($name, $value->value());
+      $value = clone $value;
+      $value->setName($name);

The other change (and the one that makes an interdiff unreadable) is the createAttributeValue is removed in favor of putting it's logic directly into offsetSet, saving a function call on the stack. So far this line looks promising - 5.11 seconds on the old code, 4.5 seconds on the new. If the cloning efficiency check is skipped the speed improvement is a more narrow 3.4 for the old code, 3.2 for the new.

After speed tests failed it became clear that cleanly converting the class over to /ArrayObject comes with too large a performance loss. This next patch falls back to the basic task of doing the other optimizations of the class outside that conversion, including the splitting off of CSS Class related code to it's own class.

Since I started over after the last patch failed speed trials an interdiff won't be useful.

@dawehner
1. Ok, reverted
2. Added (see interdiff)
3. That speeds things up noticeably, so good catch.
4. Line 156 of Drupal\Tests\Core\Template\AttributeTest calls the function with no arguments and checks to see if we have no class key afterward. So, for the moment, fulfilling the test. We can probably get away with removing the test - class doesn't render when empty whether it's set or not. I don't know which of the other tests in the system might not like that change though - I'll try it if wanted.
5. It is.
6. K, done.