Seems to be working with my crudely made custom bucket module and ad_content both thrown into the same ad block! The ad buckets are selected randomly, since their contents are also randomly selected, and that is probably the best way of ad distribution right now.
See my comment in the CI issue ( https://www.drupal.org/project/ad/issues/3525832#comment-16134384 📌 Fix failing tests / CI pipeline and clean-up code Active ).
Yes, as far as I'm aware, we are done here.
One lingering issue that I still encounter here is that, for some reason, the parameters sent over from the JS to the Controller on the page do not contain the “placement” value; even though they definitely should after some thorough checking on my side. BUT this only happens when you are admin for another reason? This baffles me quite a bit.
@anybody or @grevil, would you mind taking a look at that?
Apart from that bug, all features are implemented here.
I urge you to perform a refactoring or a search when renaming variables or functions, since you missed a few calls to them. I will refactor those quickly.
Case closed: We were only calling setIcon($icon) as the default case, but we always have to call it. Why? Otherwise getIcon() in MiconIconize returns null and because the render() function checks for that, it just returns and skips all of our set parameters!
Another success for debugging.
lrwebks → made their first commit to this issue’s fork.
@anybody and I have now established the concept that we want to implement, and I will do that here.
Re #18: Click tracking does work, so everything is working here. Click tracking is also not tracked for users with the bypass permission.
Well, I was so focused on my research within the ad_track module, that I somehow missed changing the ad content formatter from “Ad content” to “Ad content with click tracking”… So it still works. Another mystery solved.
There is one thing bugging me, though: This formatter lies within the ad_content module, but is specifically designed for the ad_track module. I think that is bad, because they are supposed to be independent and exchangeable pieces of the ad puzzle. But I also don't know if putting the formatter into the ad_track module would help any better than the current approach. @anybody, do you think this needs a change?
@anybody: As far as I'm aware it did work in 4.x, yes.
I have searched for a bit and can't seem to really grasp how the click track system is supposed to work. It uses a route and controller, which then passes the track event on to the tracker via trackClick(). So far, so good. But, the route that is used for that doesn't seem to appear anywhere or be requested/called/opened anywhere. Does 11.x require a rewrite of that system, possibly?
@grevil: Perhaps you could take a look at what is the problem here?
Everything should be operational now in !12. Unfortunately, click tracking is currently not working either way, even on 11.x, see 🐛 Click tracking doesn't work anymore Active .
As far as I know, Ad Scheduler module is fine, but missing a composer.json with the dependency for the “scheduler” module. That's why PHPUnit can't find the Scheduler classes. I actually added that in the 11.x meta issue, but it was removed again before merge for some reason.
I'll quickly resolve the MC.
Update hook added.
Since there were no changes to the docs yet, could you at least provide a comprehensive walkthrough or required code snippets as a comment here, so that we could continue with our plugin implementation?
This is more of a meta issue, as far as I can see, since all requirements listed here are already being tackled in the linked issues. So there will probably be no work to be done here directly.
Everything works as expected now!
lrwebks → changed the visibility of the branch 3399905-no-ad-bucket-handling to hidden.
I suppose that this issue would also resolve ✨ Add roles permission Bypass ADs tracking (Track ADs exclude for whis roles.) Active ? But I'm not sure, so I left a more detailed question regarding our progress in this issue there.
Question (mainly directed at @anybody): 📌 Integrate ad content type level permissions using "permission_callbacks" and "AccessControlHandler" Active will introduce a new permission called “View ads”. Only roles with that permission will see ads on the live site. Would this solve this issue? Or is a separate “Bypass ad tracking” permission sill necessary? I personally don't know why someone would want ads shown to them, but not being tracked, but what do I know.
Everything should be working as expected now. Edit, create and Delete permissions for any and all ad types as well as a new permission to choose to whom ads will be shown.
Working on a fix for that issue now.
Per-entity-type permission granularity is now working!
There is still one problem that I am facing right now:
I have added a view ads permission, as we will definitely need that later on. However, the check for that permission in AdContentAccessControlHandler seems to get overridden by the admin permission for the ad content (administer ads) and I can't figure out where that happens or how to get rid of the override yet.
Ask an API suggests that a possible fix would be to implement hook_node_view_alter(), but that seems like a hack to me.
@grevil what do you think?
I have opened the following issues for old features:
✨
Go further with the statistics view
Active
✨
Re-add desired features from older module versions
Active
The module page is now rewritten in accordance with the new README and contains a few screenshots.
Unset the controls setting and unified the summary as discussed with @grevil and @anybody. (Unifying the summary was necessary since the summary array provided by FileMediaFormatterBase was unfortunately not keyed. So no dice unsetting a specific line…)
Let's also try to check if this still occurs in 11.x — as far as I'm aware, it does not generate any sort of link like that currently.
This is correct and fixed in 11.x after 📌 [META] 11.x feature updates Active is merged!
The field_image field not be present anymore in the newest version of 11.x (replaced by field_ad_image in the image_ad type), so it's fixed, I guess?
Yes, fixed in 📌 [META] 11.x feature updates Active AFAIK.
Thanks for your quick response, @jonathan1055,
indeed I was aware about the docs page that you linked, but within the list of steps there appear to be a few instructions, that seem to me like this is meant to be implemented within the Scheduler Module (like support for a module from your module's side, the way that you have done it with Commerce Product already).
Examples of what I mean:
Add extra two lines in config/schema/scheduler.schema.yml to use the saved alias for the new third party settings.
In scheduler.install add a hook_update function to load the new view.
This was the case for Media and Commerce Products (note from lrwebks: Two of the modules that scheduler supports internally, which is not what we want to achieve with our module)
Update README.md to include the new entity in the list of implementations.
Is it enough to avoid the steps which require modification of the scheduler module, for our custom plugin implementation? Or do we have to handle that differently? If so, what are the required steps?
📌 [META] 11.x feature updates Active will provide the basic implementation of the submodule, that will be finished here. I have made a support request to the scheduler maintainers for a proper guideline of implementing the scheduler plugin as the docs are a bit unclear and all over the place. ( 💬 How to integrate scheduler with custom / contrib entity types? Active )
@grevil, it is not necessary to unset the controls setting that is passed down from FileMediaFormatterBase, as we no longer actually process it. Instead, we use our new controlsDisplay setting and pass that down to the player as data-controls. So an override with no changes to the following outcome would be unnecessary.
Unfortunately, due to the structuring of the tracker settings, I believe it is simply not possible to set a default tracker:
The tracker settings themselves are in the "ad" module, while the local tracker is the "ad_track" module. This is that way because you are able to extend the module with custom trackers from other modules, which will then also appear in the select list.
So, we have three options here, all of which I don't find particularly good:
- Place the tracker settings into the "ad_track" module (Caveat: Local tracker is always present even if unwanted)
- Require the "ad_track" module to be enabled when installing "ad" (Caveat: same as above)
- Overwrite the setting value when "ad_track" is installed afterward (Caveat: would disable any custom tracker already selected and delete the data!)
Another option would be to display a message after "ad" is installed: "You need to set a tracker in the settings in order for tracking impressions to work. If you don't have a custom tracker, you should enable ad_track" or something like that, but that would only be my last resort.
@anybody, what do you think of this?
@anybody, regarding the scheduler integration: I'm still a bit unsure about how this could be achieved properly, as the simple plugin implementation does not seem to work yet. As we discussed, would you take a look?
Should work as expected now!
lrwebks → made their first commit to this issue’s fork.
@grevil: I was unable to provide the settings via JS drupalSettings, since when there were multiple instances of vidstack players on the same page, the settings of the first player were also used as the settings for all other players (usually correct, but it was messing up the differences in settings between local and remote players). By using data attributes as described in the vidstack docs injected via a twig template, I can fine-tune the settings for each individual player that is injected into the page. This will hopefully prove useful again, if we ever want/need to override the player settings on a per-entity basis.
I have now replaced all backwards compatible calls with the proper logger service via dependency injection. Would appreciate if someone took a look at it!
LGTM!
Part of our 11.x efforts, so it will be a follow-up to 📌 [META] 11.x feature updates Active .