chris burge → created an issue.
I spoke too soon. I don't believe Memcache is at fault.
The SectionComponent
class has never had a thirdPartySettings
property. It is proposed in
✨
Support third party settings for components within a section
Needs work
; however.
@hswong3i has your site previously run a patch from ✨ Support third party settings for components within a section Needs work ?
When a Layout Builder component is serialized for storage, it includes properties from its SectionComponent
object. If a site is patched to add third-party settings to to LB components and that patch is later removed, you'll get the notice you posted. This is the same error without Memcache enabled:
Deprecated function: Creation of dynamic property Drupal\layout_builder\SectionComponent::$thirdPartySettings is deprecated in Drupal\Component\Serialization\PhpSerialize::decode() (line 21 of /var/www/html/web/core/lib/Drupal/Component/Serialization/PhpSerialize.php)
I suspect if you search the database, you'll find orphaned thirdPartySettings
properties inside serialized SectionComponent
objects.
I'm running into the same notice in the issue description, and it's filling the logs every request. I can't explain it, but patch #4 fixes it. No more notices.
I missed this issue when opening ✨ Allow for RUM Manual Instrumentation Needs review , which implements this feature request in MR-5.
@godotislate - I like the approach of caching attachments in a class property. One thing I noticed when stepping through the method with a debugger is that editor_load()
still gets called every loop if the text format doesn't have an editor assigned to it.
I just pushed a commit to the MR that add an $editors
property to the class and checks if the text format has been processed yet instead of checking if $settings['editor']['formats'][$format_id]
is set. This ensures editor_load()
is only called once for each text format.
I'm not sure what kind of text coverage could be added for this MR. We can't test performance with unit tests, and the existing test coverage should ensure we haven't broken anything.
Provider docs: https://support.bluebillywig.com/advanced-topics/oembed-service/
oEmbed specs: https://oembed.com/
https://*.bbvms.com/p/*/c/*.html?inheritDimensions=true&placementOption=default
appears to be a valid resource URL.
The provider endpoint URL, as specified by bbvms.com is https://<publication>.bbvms.com/oembed/
; however, the endpoint URL cannot contain wildcards. I'm not sure how you'd do this at runtime off the top of my head. You might need to decorate the media.oembed.resource_fetcher
service with custom code and modify the ResourceFetcher::fetchResource()
method to modify the URL before the resource is fetched.
@sea2709 - That's exactly the issue. We can't change Html::cleanCssIdentifier
without causing CSS bugs.
#13
I'm wondering if the best way to solve this issue is to use a configuration flag/setting similar to the $conf['allow_css_double_underscores'] setting in D7 that allowed BEM double underscore CSS selectors.
I think the behavior should be configurable. Maybe a $setting to define additional characters?
@senzaesclusiva - Glad to hear it!
chris burge → created an issue.
@erwangel - This was fixed in 🐛 Calling static trait method is deprecated (redux) Fixed but hasn't been released yet.
@senzaesclusiva - To clarify - Is the issue resolved for you?
I'm not seeing any changes in 2.0.0 that explains the exception. We do have two prior reports of issues with Rumble.com; however. Please see #3253757-14: Unable to create the remote video → .
@mabho Could you try rebuilding cache?
chris burge → created an issue.
chris burge → created an issue.
Closing in favor of ✨ Add Drupal 11 Support; Drop support for 'ckeditor' and 'ckeditor4to5upgrade' plugin types Active /
chris burge → created an issue.
@justcaldwell - Thanks for opening this issue!
On second thought, we can just change the trait to a class.
It looks like we'll need to remove HelperTrait
and just define a procedural function in the module file.
🐛 Allow form redirects to ignore ?destination query parameter Fixed was committed and released in D10.2.0: https://www.drupal.org/node/3375113 →
Here's the code I used to attempt to implement the fix:
function layout_builder_operation_link_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
if ($form_state->getFormObject() instanceof \Drupal\layout_builder\Form\OverridesEntityForm) {
$form_state->setIgnoreDestination();
}
}
I confirmed with my IDE that ->setIgnoreDestination()
is firing; however, the destination query parameter is still being rendered. I don't believe the "fix" addresses the use case for operation links. I think we might just need to update the comment in layout_builder_operation_link_preprocess_links()
to remove the reference to the core issue we were waiting on.
Committed and released
D11 support was already added in 📌 Add Drupal 11 Support Fixed - it just needs released.
Manual testing was successful. Logs are clean.
@aaron.ferris - Thanks for jumping in and providing a solution!
chris burge → created an issue.
This issue is blocked by 📌 Drupal 11 compatibility for Features module Active .
chris burge → made their first commit to this issue’s fork.
chris burge → made their first commit to this issue’s fork.
chris burge → made their first commit to this issue’s fork.
chris burge → made their first commit to this issue’s fork.
chris burge → created an issue.
I'm good committing this. Let's release as 2.2.0.
Drupal 11 compatibility is released in 2.2.0 → .
Access checks are only necessary on content entities, not on config entities: https://www.drupal.org/node/3201242 → .
chris burge → made their first commit to this issue’s fork.
When I asked around among colleagues today, I was pointed to this blog post: https://dev.acquia.com/blog/drupalelementstyle-add-styles-drupal-media-c....
@bkosborne - Is this solution functionally equivalent to the plugin you authored?
This makes sense. I have a customer who leverages paragraphs for their site, often with a dozen or more rich text fields on given node edit page. Page load times are quite slow in these instances.
The ::getAttachments()
method belongs to the Drupal\editor\Plugin\EditorManager
class, so moving this issue to editor.module.
Static caching seems reasonable. &drupal_static()
can be used. Editors and text formats have a 1:1 relationship. Text formats have permissions and control access their editor, so I don't see any permission issue here with caching.
We'll want to make sure to preserve the behavior of hook_editor_js_settings_alter()
(i.e. $this->moduleHandler->alter('editor_js_settings', $settings);
and below should not be cached.
Needed to run tests. Maintainers should feel free to remove .gitlab-ci.yml file prior to merging and merge 📌 Switch from Drupal CI to GitLab CI Needs review separately.
@_m - Code is merged! Thanks
Chris Burge → created an issue.
@neclimdul - Sorry for flooding your inbox, but I think we're really close on this MR now.
In terms of outstanding items, there's been discussion around how to describe the settings' behaviors:
https://git.drupalcode.org/project/new_relic_rpm/-/merge_requests/5#note...
I'm open to revising documentation and relabeling. It strikes me the machine name 'auto' could be better named 'no-action'.
There was also discussion regarding the UpdateTest test class:
https://git.drupalcode.org/project/new_relic_rpm/-/merge_requests/5#note...
@phenaproxima - Thanks for the code review
Chris Burge → created an issue.
Tests are passing again - now to address the rest of the technical review.
There's too much going on in this MR to not be running tests. I added .gitlab-ci.yml. The maintainers should feel free to remove the file and commit separately in 📌 Switch from Drupal CI to GitLab CI Needs review before merging.
I'd recommend implementing hook_entity_operation_alter() in custom code.
Chris Burge → made their first commit to this issue’s fork.
There is some interaction between Drupal's internal cache system and newrelic_get_browser_timing_footer()
that I can't explain. I'm logged in as user 1. If I load any admin page, the footer JS is consistently rendered. If I visit non-admin pages, the footer JS is only rendered on the first page load after a cache rebuild. I even tried disabling the decorator and acting on the markup directly in an event subscriber, acting on the Symfony ResponseEvent
. Stashing the footer JS in the default cache bin seems to do the trick, however.
@e.ryan.schmidt@gmail.com - Is the New Relic PHP extension loaded?
A quick way to check is to navigate to the 'Status Report' page and scroll down to 'New Relic PHP Library', which should have a value of 'Exists'.
When big_pipe is enabled, HtmlResponseAttachmentsProcessor->processAttachments()
gets called several times, which means newrelic_get_browser_timing_footer()
gets called multiple times, which means all calls after the initial call return an empty string. The latest MR code uses drupal_static()
to cache the return value of newrelic_get_browser_timing_footer()
.
In manual testing, I was able to reproduce this behavior with Big Pipe enabled; however, enabling Big Pipe in InstrumentationTest
didn't result in newrelic_get_browser_timing_footer()
being called multiple times, so I added an event subscriber to the new_relic_rpm_intstrumentation_test module to simulate the behavior.
I received some feedback out of thread that I investigated. Here's a summary of the feedback:
The header JS renders without issue and consistently; however, the footer JS only renders on the first page load immediately after a Drupal cache rebuild.
The header JS is inserted using hook_page_attachments()
, complete with a cache dependency. Simply put, it works as designed. Some wizardry is needed to get the footer JS inserted below other JS. What appears to be happening is that newrelic_get_browser_timing_footer()
is getting called multiple times during the same request. If either newrelic_get_browser_timing_header()
or newrelic_get_browser_timing_footer()
are called more than once during the same request, they'll return an empty string on subsequent requests. I verified this manually.
I'm debating abandoning the service decoration approach and using an event subscriber to alter the DOM directly. I'm also considering modifying ExtensionAdapter::newrelic_get_browser_timing_header()
and ExtensionAdapter::newrelic_get_browser_timing_footer()
to leverage drupal_static()
.
Last code push takes a different approach (still using a decorator); however, it uses str_replace()
to insert the rendered footer JS into the rendered markup before returning $this->decorated->processAttachments($response)
. This approach avoids the code duplication issue.
Just pushed code to properly decorate the service. Unfortunately, seven of the methods on the class are protected, so I had to copy/paste them all over to the decorator. (The decorator can't access protected methods on the inner service.) I also added a test to ensure the decorator doesn't break core functionality.
@neclimdul - Drupal's APIs don't provide a way to insert the New Relic footer JS below Javascript inserted by Drupal's Libraries API. NR's docs for manual instrumentation state:
As the very last thing before the closing
</body>
tag in the document, or as close to it as possible, insert the return value ofnewrelic_get_browser_timing_footer().
Below is some sample HTML output from a fresh Drupal site. At the very top is where markup is inserted with hook_page_bottom()
. At the bottom is code inserted with HtmlResponseAttachmentsProcessorDecorator::processAssetLibraries()
.
<div>Inserted with hook_page_bottom()</div>
<script type="application/json" data-drupal-selector="drupal-settings-json">{"path":{"baseUrl":"\/","pathPrefix":"","currentPath":"node","currentPathIsAdmin":false,"isFront":true,"currentLanguage":"en"},"pluralDelimiter":"\u0003","suppressDeprecationErrors":true,"ajaxPageState":{"libraries":"eJx9UFuOgzAMvFAgZ9iTVCaZLdaGGNkGltsvRYVqkdqf2PPwKBrKA9ebi5SOND5n6woEeiu1vcxQro7q_237tNPMSWro-H4beUQ8lrCxjl-fqMSs00ilfTFN4fpjny3P-M2kOGRKzjP26yBlW1XiN5BPUGnmOzlLbQxbWCZdT3GUBYrcdGvTFUmvCANp6ptKqrJc2YUzgvWiniY_PnLgYKs5htiRIVxavOIWlmjE16PKMBn0CHvse4sWZsZicX_bQfJU8AduD62d","theme":"olivero","theme_token":null},"ajaxTrustedUrl":{"\/search\/node":true},"bigPipePlaceholderIds":{"callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages\u0026args%5B0%5D\u0026token=_HAdUpwWmet0TOTe2PSiJuMntExoshbm1kh2wQzzzAA":true,"callback=announcements_feed.lazy_builders%3ArenderAnnouncements\u0026\u0026token=m--BNYDqgTao0j2QoLFf1TwfQN-Bm9xPlckw9QAC1h4":true,"callback=user.toolbar_link_builder%3ArenderToolbarLinks\u0026\u0026token=QPmvukHqpEJJ4rYEzUUFx0ERxrtfmQ9TMOp_hvyLHEk":true,"callback=user.toolbar_link_builder%3ArenderDisplayName\u0026\u0026token=XGROnxBLjNjdNZJ_VcOhVZtenx2tDkIKPfojS_uZFXA":true,"callback=shortcut.lazy_builders%3AlazyLinks\u0026\u0026token=5-XBI-QHgyU_l7Bu0FizHR7YBqK2bTVTBFF8Z0DVTis":true},"toolbar":{"breakpoints":{"toolbar.narrow":"only screen and (min-width: 16.5em)","toolbar.standard":"only screen and (min-width: 38.125em)","toolbar.wide":"only screen and (min-width: 61em)"},"subtreesHash":"DvNlpBZa2F73VL_ZCBelzSCBD08FX9eADzsmGiR_J_o"},"user":{"uid":"1","permissionsHash":"e04100661cd70d74a85410e24e7c918449cfc6bbc6d71bae1bf7fb8fa4d2bd8f"}}</script>
<script src="/core/assets/vendor/jquery/jquery.min.js?v=3.7.1"></script>
<script src="/core/assets/vendor/underscore/underscore-min.js?v=1.13.6"></script>
<script src="/core/assets/vendor/once/once.min.js?v=1.0.1"></script>
<script src="/core/assets/vendor/backbone/backbone-min.js?v=1.5.0"></script>
<script src="/core/misc/drupalSettingsLoader.js?v=10.2.6-dev"></script>
<script src="/core/misc/drupal.js?v=10.2.6-dev"></script>
<script src="/core/misc/drupal.init.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/contextual.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/models/StateModel.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/views/AuralView.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/views/KeyboardView.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/views/RegionView.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/views/VisualView.js?v=10.2.6-dev"></script>
<script src="/core/assets/vendor/tabbable/index.umd.min.js?v=6.2.0"></script>
<script src="/core/misc/progress.js?v=10.2.6-dev"></script>
<script src="/core/assets/vendor/loadjs/loadjs.min.js?v=4.2.0"></script>
<script src="/core/misc/debounce.js?v=10.2.6-dev"></script>
<script src="/core/misc/announce.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/navigation-utils.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/checkbox.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/messages.js?v=10.2.6-dev"></script>
<script src="/core/misc/message.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/message.theme.js?v=10.2.6-dev"></script>
<script src="/core/misc/ajax.js?v=10.2.6-dev"></script>
<script src="/core/misc/active-link.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/navigation.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/second-level-navigation.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/nav-resize.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/search.js?v=10.2.6-dev"></script>
<script src="/core/misc/displace.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/toolbar.menu.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/toolbar.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/models/MenuModel.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/models/ToolbarModel.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/views/BodyVisualView.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/views/MenuVisualView.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/views/ToolbarAuralView.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/views/ToolbarVisualView.js?v=10.2.6-dev"></script>
<script src="/core/misc/tabbingmanager.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/contextual.toolbar.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/toolbar/models/StateModel.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/toolbar/views/AuralView.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/toolbar/views/VisualView.js?v=10.2.6-dev"></script>
<script src="/modules/contrib/admin_toolbar/js/admin_toolbar.js?sf2suw"></script>
<script src="/modules/contrib/admin_toolbar/js/jquery.hoverIntent.js?sf2suw"></script>
<script src="/modules/contrib/admin_toolbar/js/admin_toolbar.hoverintent.js?sf2suw"></script>
<script src="/core/modules/toolbar/js/escapeAdmin.js?v=10.2.6-dev"></script>
<script src="/core/modules/big_pipe/js/big_pipe.js?v=10.2.6-dev"></script><div id="drupal-live-announce" class="visually-hidden" aria-live="polite" aria-busy="false">Tray "Administration menu" opened.</div>
<script type="text/javascript">console.log("Inserted with HtmlResponseAttachmentsProcessorDecorator::processAssetLibraries()");</script>
Using a decorator preserves the original service and also allows other code to decorate the html_response.attachments_processor
, as well.
@jack.minster may wish to weigh in, as well.
@pieterdtt - Thanks for patch #21. I incorporated it into the MR. Based on how New Relic's auto-instrumentation inserts the header JS into <head>
, I think the best thing is to always insert the JS there (i.e. no need for placement configuration).
@jack.minster - Could you try out the latest code push to the MR? The header JS is now rendered in <HEAD>
near the very top. I also abandoned the library hack for the footer. In HtmlResponseAttachmentsProcessorDecorator
, the MR is now modifying the ::processAssetLibraries()
method to insert the JS into scripts_bottom
, bypassing the #attached
types restrictions of ::processAttachments()
.
@jack.minster - Thanks for that feedback. I'll look into the footer JS insertion issue. I've been doing some work with the test coverage for that piece. One change I've made (but haven't pushed yet) is to execute JS that adds an element to the DOM in the test. The current test results in a false positive.
I do have some work in progress that moves the header JS into <head>
.
@jack.minster - Would you mind sending me a DM on my Drupal.org contact form with your contact info and Acquia application docroot?
When I look at a Drupal site where RUM auto-instrumentation is used, I do find the header JS in <head>
. I think that's the way to go.
GitLab CI isn't configure for this project yet, so automated tests won't run, but tests are passing locally:
$ ../vendor/phpunit/phpunit/phpunit -c core/ modules/contrib/new_relic_rpm/tests/src/ --verbose
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.
Runtime: PHP 8.1.27
Configuration: /var/www/html/web/core/phpunit.xml.dist
Testing /var/www/html/web/modules/contrib/new_relic_rpm/tests/src
................... 19 / 19 (100%)
Time: 00:04.735, Memory: 8.00 MB
OK (19 tests, 33 assertions)
HTML output was generated
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_Functional_AdminUiTest-9-53556339.html
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_Functional_AdminUiTest-10-53556339.html
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_Functional_AdminUiTest-11-53556339.html
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_Functional_AdminUiTest-12-53556339.html
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_FunctionalJavascript_InstrumentationTest-5-95610470.html
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_FunctionalJavascript_InstrumentationTest-6-95610470.html
Remaining self deprecation notices (3)
1x: user_role_names() is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use \Drupal\user\Entity\Role::loadMultiple() and, if necessary, an inline implementation instead. See https://www.drupal.org/node/3349759
1x in AdminUiTest::testSettingsPage from Drupal\Tests\new_relic_rpm\Functional
1x: user_roles() is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use \Drupal\user\Entity\Role::loadMultiple() and, if necessary, an inline implementation instead. See https://www.drupal.org/node/3349759
1x in AdminUiTest::testSettingsPage from Drupal\Tests\new_relic_rpm\Functional
1x: Method "Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents()" might add "array" as a native return type declaration in the future. Do the same in implementation "Drupal\new_relic_rpm\EventSubscriber\RoutingTransactionNameSubscriber" now to avoid errors or add an explicit @return annotation to suppress this message.
1x in RoutingTransactionNameSubscriberTest::testSetsTransactionNameForAllRoutes from Drupal\Tests\new_relic_rpm\Unit\EventListener
@jack.minster - I'll be interested in feedback you get from your New Relic performance experts.
Re the footer JS, I found that you can control library weights by decorating the html_response.attachments_processor
service. I need to clean up my local code, and then I'll push it to the MR.
@jack.minster - Thanks for the feedback! I'm glad to hear the MR code is working for you all.
Re placement of the header JS, I do see that in their documentation, too; however, when I look at their WordPress and Drupal 7 examples, they're calling the newrelic_get_browser_timing_header()
inside <body>
.
The placement of footer JS is trickier. The MR currently uses hook_page_bottom()
to insert the footer JS; however, most JS is loaded after that before the </body>
tag. I've tried various methods to address this without success:
1. Call hook_page_attachments()
to add a script to scripts_bottom
:
function new_relic_rpm_page_attachments(array &$attachments) {
$attachments['#attached']['scripts_bottom'][] = [
[
'#tag' => 'script',
'#attributes' => [
'type' => 'text/javascript',
],
'#value' => 'console.log("test");',
],
'new_relic_rpm_rum_footer',
];
}
This isn't allowed:
LogicException: You are not allowed to use scripts_bottom in #attached. in Drupal\Core\Render\HtmlResponseAttachmentsProcessor->processAttachments() (line 150 of core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php).
2. Register a library, pass the return value of newrelic_get_browser_timing_footer()
via drupalSettings
to a JS file, and then pass that value into eval()
:
new_relic_rpm.libraries.yml:
rum_footer:
js:
js/new-relic-browser-monitoring.js: {}
dependencies:
- core/drupalSettings
new_relic_rpm.module:
function new_relic_rpm_page_attachments(array &$attachments) {
$attachments['#attached']['library'][] = 'new_relic_rpm/rum_footer';
$attachments['#attached']['drupalSettings']['rum_footer']['markup'] = 'console.log("test")';
}
js/new-relic-browser-monitoring.js:
(function (Drupal, drupalSettings) {
Drupal.behaviors.NRRumFooter = {
attach: function (context, settings) {
once('myBehavior', 'html').forEach(function (element) {
eval(drupalSettings.rum_footer.markup);
})
}
};
})(Drupal, drupalSettings);
This executes the JS code in scripts_bottom
; however, we can't weight the library with a high value to push the JS insertion to the bottom.
rum_footer:
js:
js/new-relic-browser-monitoring.js:
weight: 999
dependencies:
- core/drupalSettings
Exception:
UnexpectedValueException: The new_relic_rpm/rum_footer library defines a positive weight for 'js/new-relic-browser-monitoring.js'. Only negative weights are allowed (but should be avoided). Instead of a positive weight, specify accurate dependencies for this library. in Drupal\Core\Asset\LibraryDiscoveryParser->buildByExtension() (line 197 of core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php).
I'm at a bit of a loss. It may not be possible to get that JS code immediately before </body>
using Drupal's API. It may be necessary to customize html.twig.html.
That said, if someone could weigh in regarding the placement of the footer JS before/after all of the libraries JS, that would be helpful. It strikes me that we may not get valid browser monitoring results if the footer JS is inserted before the rest of the JS on the page.
Here's a screenshot of the requirements error message, which is added in the MR:
We could run the same compatibility check as in new_relic_rpm_requirements()
and return a warning string if the site is configured to allow auto-instrumentation on D10.2.0+.
@neclimdul - Thanks for reviewing this merge request!
1. Are there possibly additional problems with caching with how manual adds adds code? Specifically, is the "header" being added unique to the request? I see the settings tag but should this be a lazy callback or have additional tags to avoid page/render caches?
I'm not aware of any caching-related issues. The two functions, newrelic_get_browser_timing_header()
and newrelic_get_browser_timing_footer()
are unaware of anything related to the request or the response. In the 7.x branch of this module, they are implemented without any accommodations for caching.
2. In the post update, should it take the Drupal version into account and migrate to manual instead of migrating to a auto that behaves like manual with a not immediately obvious warning? Or maybe it should warn the user during the update? Unsure.
All that the post_update function does is preserve the same behavior following the update. The merge request does add a check to new_relic_rpm_requirements()
that alerts site owners about the issue of auto-instrumentation and D10.2.0+.
@vipin.mittal18 - Thank you for your review.
It's not explicitly stated above, but here's the expected behavior for each of the three settings:
Pre 10.2.0
Disable browser monitoring - No JS inserted
Allow auto-instrumentation - JS inserted (assuming auto-instrumentation is enabled on server)
Manual instrumentation - JS inserted
10.2.0+
Disable browser monitoring - No JS inserted
Allow auto-instrumentation - No JS inserted (because auto-instrumentation is incompatible with D10.2.0+)
Manual instrumentation - JS inserted
Is Features configured to export permissions with roles? It sounds like that is the issue.
@Shubham_Kumar - Thanks for testing the module out!
Chris Burge → made their first commit to this issue’s fork.
It sounds like the behavior only presented on existing blocks, which would be the referenced issue; however, the fact that clearing cache and trying a different browser didn't work isn't consistent with prior reports. This module uses core APIs to render the links, so, if there is an issue, it would most probably be a core issue.
I'm wondering if this issue might be the same as #3193067: Manage attributes link not displaying for existing blocks → . @eit2103, can you try the same steps in a fresh browser session (e.g. incognito mode)?
Can you help me understand what you mean by "standard layout builder pages" and by editing "the layout of individual content items"? A screenshot of the latter might be helpful.
@Erik_MC - If your team would be able to test the merge request and report back, that would be helpful in moving the issue forward. The MR is coded based on New Relic's documentation; however, I don't have a New Relic instance to test against myself.
Chris Burge → created an issue.
Chris Burge → created an issue.
Unit tests are passing. The cspell, phpcs, and phpstan failures can be deferred.
Chris Burge → created an issue.
Thanks, @iler!